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

[Evan Cheng]

On Aug 2, 2009, at 6:01 AM, Jakob Stoklund Olesen wrote:

> Implicitly defined registers are marked with <undef>, and may be  
> used by a MachineOperand without being live. LowerSubregs does not  
> yet know about the <undef> flag, causing PR4657 in this code:
>
> %Q1<def> = INSERT_SUBREG %Q1<undef>, %D1<kill>, 5
> %Q1<def> = INSERT_SUBREG %Q1, %D0<kill>, 6
> ...
> subreg: CONVERTING: %Q1<def> = INSERT_SUBREG %Q1<undef>, %D1<kill>, 5
> subreg: %D2<def> = FCPYD %D1, 14, %reg0
>
> The original INSERT_SUBREG instruction defines %Q1, the replacement  
> FCPYD does not. The <undef> flag means that %Q1 may not be live, and  
> the register scavenger asserts later when %Q1 is used.

Ok, that's bad.

>
> The attached patch teaches LowerSubregs about the <undef> flag:
>
> * Add a %Q1<imp-def> operand to the inserted copy instruction. This  
> makes sure that the super register is live.

Ok.

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

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

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

> * 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>
         // ...
         ///    =  EAX


>
> With the patch, the LowerSubregs replacement looks like this:
>
> subreg: CONVERTING: %Q1<def> = INSERT_SUBREG %Q1<undef>, %D1<kill>, 5
> subreg: %D2<def> = FCPYD %D1<kill>, 14, %reg0, %Q1<imp-def>
>
> The %D1<kill> flag has been correctly transferred and %Q1<imp-def>  
> was added. This fixes PR4657.

Great. Thanks.


>
>
> I am not completely sure it is a good idea to insert IMPLICIT_DEF  
> instructions in LowerSubregs. The problem is this instruction:
> %Q1<def> = INSERT_SUBREG %Q1<undef>, %D2<kill>, 5
> Since %D2 is subreg #5 of Q1 it is a noop and can be removed.  
> However, that would mean the %Q1 is no longer considered live after  
> the instruction, causing more regscavenger asserts. In the patch, I  
> replace INSERT_SUBREG with this instruction:
> %Q1<def> = IMPLICIT_DEF %Q1<imp-kill>
> It is a noop that makes sure %Q1 is live. It think it is a misuse of  
> IMPLICIT_DEF, but I don't know what else to do. Suggestions welcome.

The ideal solution is to add <undef> to the following uses of Q1 (and  
it's sub-registers). But that'll probably require scanning the  
following instructions. That's not a good idea either.

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.

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()?

Evan

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