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

[Evan Cheng]

On Aug 2, 2009, at 4:01 PM, Jakob Stoklund Olesen wrote:

>
> 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.


Yeah. I don't think it's a good option. Turning it into an  
IMPLICIT_DEF is perfectly ok. When LiveIntervalAnalysis add <undef>  
marks on uses of IMPLICIT_DEF, it also leaves those which have uses in  
other BBs alone.

>
>>
>>> * 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.

Does it ever happen?

>
> 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.

Yes.

>
>>
>>> * 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.

Is this it?
> -    assert(isUsed(Reg) && "Using an undefined register!");
> +    assert((MO.isImplicit() || isUsed(Reg)) && "Using an undefined

But isn't it true you would ever introduce an implicit kill during  
insert_subreg lowering when the super-register was already live? That  
means isUsed(Reg) should be true. I really have trouble with this  
change as it would allow some real bugs through.

The example you gave is this:
> %R1<def,dead> = INSERT_SUBREG %R1<undef>, %R0L<kill,undef>, 1

This should have been translated to an implicit_def. If that's done,  
do we still have to relax the scavenger?

>
>>
>>> * 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.

Yes, it's very difficult to model.

>
> [...]
>
>>> %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.

This is discussed in more detail earlier.

Evan

>
> /jakob
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090802/a2038300/attachment.html>