llvm dev - Oct 2017 - Removing the register block in MIR

The MIR format currently has a short-hand syntax for declaring vreg
classes and banks in the function body so you can write something like
this:

  name: foo
  body: |
    %3:gpr(s64) = ...

rather than the much more verbose and awkward:

  name: foo
  registers:
    - { id: 3, class: gpr }
  body: |
    %3(s64) = ...

I'd like to make this shorthand the only way to do this. There are a few
things that need to be handled here:

- We should only print the class on defs, not all uses. This is
  sufficient to be unambiguous, easy to be consistent, and avoids
  getting in the way of readability.

- We'll still need to track the preferred-register elsewhere, so we'll
  still need a block for those. They're used far less often than
  classes though, so I think its fine to just let them have their own
  block when any of them are set.

- Basically every single MIR test will need to be updated, and its
  awkward to do automatically unless the test is already using the
  update_mir_test_checks script.

I plan to implement this in a couple of steps:

1. Teach the MIRPrinter to print the regclass and update test checks
2. Modify existing tests to never provide the registers block
3. Move preferred-registers to their own block and remove the registers block

A patch for step 1 is attached (with the other 241 test updates omitted
for brevity). I don't expect this direction to be contentious, but since
the change will touch so many files I'll wait until Monday or Tuesday to
start committing in case anyone has any concerns.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mir-print-vregs.patch
Type: text/x-patch
Size: 5056 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171020/8649af80/attachment.bin>

> On Oct 20, 2017, at 5:07 PM, Justin Bogner via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> The MIR format currently has a short-hand syntax for declaring vreg
> classes and banks in the function body so you can write something like
> this:
> 
>  name: foo
>  body: |
>    %3:gpr(s64) = ...
> 
> rather than the much more verbose and awkward:
> 
>  name: foo
>  registers:
>    - { id: 3, class: gpr }
>  body: |
>    %3(s64) = ...
> 
> I'd like to make this shorthand the only way to do this. There are a
few
> things that need to be handled here:
Yes not using the registers list is the way to go.
> 
> - We should only print the class on defs, not all uses. This is
>  sufficient to be unambiguous, easy to be consistent, and avoids
>  getting in the way of readability.
We could in principle have no def if:
- A vreg is completely unused. I don’t have a good reason right now we should
serialize it then, so this is probably fine.
- A vreg with no def but uses marked undef. This is also not particularily
useful but legal and I could imagine this happening. Should be easy enough to
catch this special case in the printer though; something like print regclass on
all uses when there isn’t a single def.
> 
> - We'll still need to track the preferred-register elsewhere, so
we'll
>  still need a block for those. They're used far less often than
>  classes though, so I think its fine to just let them have their own
>  block when any of them are set.
Fine with me.
> 
> - Basically every single MIR test will need to be updated, and its
>  awkward to do automatically unless the test is already using the
>  update_mir_test_checks script.
> 
> I plan to implement this in a couple of steps:
> 
> 1. Teach the MIRPrinter to print the regclass and update test checks
That would be great. Ahmed had started working on this a while ago, but I think
it got stuck or was interrupted by higher priority things...
> 2. Modify existing tests to never provide the registers block
> 3. Move preferred-registers to their own block and remove the registers
block
> 
> A patch for step 1 is attached (with the other 241 test updates omitted
> for brevity). I don't expect this direction to be contentious, but
since
> the change will touch so many files I'll wait until Monday or Tuesday
to
> start committing in case anyone has any concerns.
I think it’s a great thing to do.

- Matthias

Matthias Braun <mbraun at apple.com> writes:
>> On Oct 20, 2017, at 5:07 PM, Justin Bogner via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> The MIR format currently has a short-hand syntax for declaring vreg
>> classes and banks in the function body so you can write something like
>> this:
>> 
>>  name: foo
>>  body: |
>>    %3:gpr(s64) = ...
>> 
>> rather than the much more verbose and awkward:
>> 
>>  name: foo
>>  registers:
>>    - { id: 3, class: gpr }
>>  body: |
>>    %3(s64) = ...
>> 
>> I'd like to make this shorthand the only way to do this. There are
a few
>> things that need to be handled here:
>
> Yes not using the registers list is the way to go.
Thanks! I've gone ahead and committed this change now.
>> - We should only print the class on defs, not all uses. This is
>>  sufficient to be unambiguous, easy to be consistent, and avoids
>>  getting in the way of readability.
>
> We could in principle have no def if:
> - A vreg is completely unused. I don’t have a good reason right now we
> should serialize it then, so this is probably fine.
> - A vreg with no def but uses marked undef. This is also not
> particularily useful but legal and I could imagine this
> happening. Should be easy enough to catch this special case in the
> printer though; something like print regclass on all uses when there
> isn’t a single def.
This is a good point. I went ahead and printed the regclass on all uses
when there are no defs. If we ever make this a verifier error we could
undo that part.
>> - We'll still need to track the preferred-register elsewhere, so
we'll
>>  still need a block for those. They're used far less often than
>>  classes though, so I think its fine to just let them have their own
>>  block when any of them are set.
> Fine with me.
>
>> 
>> - Basically every single MIR test will need to be updated, and its
>>  awkward to do automatically unless the test is already using the
>>  update_mir_test_checks script.
>> 
>> I plan to implement this in a couple of steps:
>> 
>> 1. Teach the MIRPrinter to print the regclass and update test checks
>
> That would be great. Ahmed had started working on this a while ago,
> but I think it got stuck or was interrupted by higher priority
> things...
Yep, I spoke with Ahmed. I think the sheer magnitude of the test update
kept him from finishing this.
>> 2. Modify existing tests to never provide the registers block
>> 3. Move preferred-registers to their own block and remove the registers
block
>> 
>> A patch for step 1 is attached (with the other 241 test updates omitted
>> for brevity). I don't expect this direction to be contentious, but
since
>> the change will touch so many files I'll wait until Monday or
Tuesday to
>> start committing in case anyone has any concerns.
>
> I think it’s a great thing to do.
>
> - Matthias