[llvm-commits] [PATCH] Fix Bug 4657: register scavenger asserts with subreg lowering

[Jakob Stoklund Olesen]

On 02/08/2009, at 21.47, Evan Cheng wrote:

>
> On Aug 2, 2009, at 6:01 AM, Jakob Stoklund Olesen wrote:
>> * Don't eliminate an identity copy when the source register is
>> <undef>. Instead, replace INSERT_SUBREG with an IMPLICIT_DEF
>> instruction.
>
> Can we change this to <undef> of the subsequent use(s)? Or is it too
> much work?

That would be preferable, but to be honest, I don't know how to find  
all the uses. Are we even sure they are in the same MBB? Would I have  
to modify live-in lists on successor blocks as well?

LowerExtract() just scans the MBB:

     // Find the kill of the destination register's live range, and  
insert
     // a kill of the source register at that point.
     if (MI->getOperand(1).isKill() && !MI->getOperand(0).isDead())
       for (MachineBasicBlock::iterator MII =
              next(MachineBasicBlock::iterator(MI));
            MII != MBB->end(); ++MII)
         if (MII->killsRegister(DstReg, &TRI)) {
           MII->addRegisterKilled(SuperReg, &TRI, /*AddIfNotFound=*/ 
true);
           break;
         }

Is that correct? DstReg may not be killed in the MBB - it could be  
live-out. If that happens, Superreg won't be killed either. I don't  
think that is actually a problem.

>
>> * When replacing %Q1<def, dead> = INSERT_SUBREG ..., add an <imp-
>> kill> of the super register.
>
> I am not sure if I understand this? Can you show me an example?

The test case CodeGen/Blackfin/printf2.ll produces:

	%R1<def,dead> = INSERT_SUBREG %R1<undef>, %R0L<kill,undef>, 1
	ADJCALLSTACKDOWN 12, %SP<imp-def>, %SP<imp-use>
	CALLa <ga:printf>, %R0<imp-def,dead>, %R1<imp-def,dead>, %R2<imp- 
def,dead>, ...
	ADJCALLSTACKUP 12, 0, %SP<imp-def>, %SP<imp-use>
	%R0<def> = LOADimm7 0
	RTS %R0<imp-use,kill>, %RETS<imp-use>

Undef everywhere. Then we run LowerSubregs:

subreg: CONVERTING: %R1<def,dead> = INSERT_SUBREG %R1<undef>,  
%R0L<kill,undef>, 1
subreg: %R1L<def,dead> = SLL16i %R0L, 0, %AZ<imp-def>, %AN<imp-def>,  
%V<imp-def>, %VS<imp-def>

Notice that if %R1 were live, it would be killed by the INSERT_SUBREG  
but not by the replacement SLL16i. This is a bad example because %R1  
is marked <undef>. Pretend that it isn't. LowerInsert already  
transfers the <dead> flag:

     // Transfer the kill/dead flags, if needed.
     if (MI->getOperand(0).isDead())
       TransferDeadFlag(MI, DstSubReg, TRI);

It just forgets to kill the super-register. I would add %R1<imp- 
use,kill> to the end of SLL16i to make sure the super-register is dead.

This example actually raises another issue:

%R1<def,dead> = INSERT_SUBREG undef, undef

can be eliminated, and

%R1<def> = INSERT_SUBREG undef, undef

can be replaced with IMPLICIT_DEF. Even when you would normally insert  
a sub-register copy.

>
>> * Fix a bug in transferring the kill flag from InsReg: InsReg is
>> operand #2, not #1.
>>
>> In the regscavenger:
>>
>> * Allow imp-use of a dead register.
>
> I don't get this part. Is this using an implicit_def?

It would allow SLL16i ..., %R1<imp-use,kill> even when %R1 is dead. It  
mirrors the "double define with imp-def is legal" rule.

>
>> * Allow redefining a sub-register of a live super register.
>
> To be precise, it should be modeled as a use and def. See
> LiveVariables.cpp
>
>         // The larger register is previously defined. Now a smaller
> part is
>         // being re-defined. Treat it as read/mod/write if there are
> uses
>         // below.
>         // EAX =
>         // AX  =        EAX<imp-use,kill>, EAX<imp-def>

I see. I will have to update the machine code verifier as well.

The exact meaning of the kill/def/undef/imp flags is quite hard to get  
right. Particularly the subreg stuff is difficult. It may be a good  
idea to document it in great detail, and the make sure the machine  
code verifier and scavenger agree.

[...]

>> %Q1<def> = INSERT_SUBREG %Q1<undef>, %D2<kill>, 5

> Are you saying in this case there isn't a matching q1 = insert_subreg
> %q1, x, 6? I think using implicit_def is perfectly ok then. Part of Q1
> is implicitly defined.

Sometimes there is, sometimes there isn't. Blackfin implements anyext  
like this:

def : Pat<(i32 (anyext D16L:$src)),
           (INSERT_SUBREG (i32 (IMPLICIT_DEF)),
                          (COPY_TO_REGCLASS D16L:$src, D16L),
                          bfin_subreg_lo16)>;

The high part of the register is left undefined for a true "anyext".  
When INSERT_SUBREG is lowered, it still must define the full register.

Example: (i32 (anyext (ONES D:$src))) becomes:

%R1.L<def> = ONES %R0<kill>
%R1<def> = INSERT_SUBREG %R1<undef>, %R1.L<kill>

This is perfectly fine until LowerSubregs removes INSERT_SUBREG  
because it is an identity copy. Then %R1 is no longer live. Actually,  
it seems a bit dodgy to propagate %R1<undef> to all uses just because  
it is the result of an "anyext".


>
> The patch looks good except for this part:
>
>
> -    assert(isUsed(Reg) && "Using an undefined register!");
> +    assert((MO.isImplicit() || isUsed(Reg)) && "Using an undefined
> register!");
>
> Do you mean MO.isUndef()?

No, this is the "Allow imp-use of a dead register" rule discussed  
above. Perhaps the rule should be "Allow imp-kill of a dead register"?

assert(((MO.isImplicit() && MO.isKill()) || isUsed(Reg)) && .... ?

I think we need a way to say "make sure this register is killed, dead  
or alive". I have been using <imp-kill> for that purpose.

/jakob