[llvm-commits] [llvm] r77989 - in /llvm/trunk: lib/CodeGen/LowerSubregs.cpp lib/CodeGen/MachineInstr.cpp test/CodeGen/ARM/2009-08-02-RegScavengerAssert-Neon.ll

[Evan Cheng]

On Aug 4, 2009, at 2:13 AM, Jakob Stoklund Olesen wrote:

> Evan Cheng <evan.cheng at apple.com> writes:
>
>> On Aug 3, 2009, at 11:05 PM, Jakob Stoklund Olesen wrote:
>>
>>>
>>> On 03/08/2009, at 23.40, Evan Cheng wrote:
>>>
>>>>> Finally, clear the undef flag in MachineInstr::addRegisterKilled.
>>>>> Undef implies dead and kill implies live, so they cant both be
>>>>> valid.
>>>>
>>>> That's not necessary. Undef doesn't implies dead. It just means its
>>>> garbage and it has no liveness.
>>>
>>> A zombie!?
>>>
>>> I am not sure what you mean.
>>> Do you mean that a register can have three states: dead-live-undef?
>>> Or do you mean that an undef operand can be both dead or live, both
>>> are OK?
>>
>> Undef operands are "don't care". :-) It would have been cleaner to
>> have an Undef machineoperand rather than a register machineoperand
>> that's undef. But we still need to assign registers to them so the
>> instructions can be translated to machine code.
>
> OK, here is the example that caused me to make that change:
>
> llvm-as < Blackfin/i8mem.ll | Debug/bin/llc -march=bfin -debug
>
> We have this code:
>
> 	%R0<def> = LOAD32p_8z %P0<kill>, Mem:LD(1,1) [i8_l + 0]
> 	%R0L<def> = EXTRACT_SUBREG %R0<kill>, 1
> 	%R0<def> = INSERT_SUBREG %R0<undef>, %R0L<kill>, 1
>
> The code is stupid because of missing Blackfin peepholes. Please  
> ignore
> that. It should be correct.
>
> Now look what happens without my fix:
>
> ********** LOWERING SUBREG INSTRS **********
> ********** Function: i8_ls
> subreg: CONVERTING: %R0L<def> = EXTRACT_SUBREG %R0<kill>, 1
> subreg: eliminated!
> subreg: killed here: %R0<def> = INSERT_SUBREG %R0<undef>,  
> %R0L<kill>, 1
>
> LowerExtract moves the %R0<kill> flag to the INSERT_SUBREG
> below. Because INSERT_SUBREG is a two-addr instruction, the kill  
> flag on
> %R0 is implicit. (I fixed that last week).
>
> Then:
>
> subreg: CONVERTING: %R0<def> = INSERT_SUBREG %R0<undef>, %R0L<kill>, 1
> subreg: %R0<def> = IMPLICIT_DEF %R0L<imp-use,kill>
>
> And that is wrong because LowerInsert assumed %R0 was dead. We get a
> double-def of %R0.
>
> When the <undef> flag is removed by MachineInstr::addRegisterKilled,  
> we
> get this instead:
>
> subreg: CONVERTING: %R0<def> = INSERT_SUBREG %R0, %R0L<kill>, 1
> subreg: eliminated!
>
> Which is correct.
>
> With your semantics for <undef>, I think we would need to do this:
>
> subreg: CONVERTING: %R0<def> = INSERT_SUBREG %R0<undef>, %R0L<kill>, 1
> subreg: %R0<def> = IMPLICIT_DEF %R0<imp-kill,undef>
>
> This won't work now because the regscavenger ignores all <undef>
> operands, and we still get the double-def assert. That can be fixed,  
> of
> course.

I agree not removing the <undef> here is correct. We need a way to  
model r0 is killed before the def.

>
> Note that if we do this, %R0<imp-kill,undef> means "kill %R0, dead or
> alive". That is what you wanted to avoid, I think. It also means that
> RegScavenger::backward probably can't work properly - Would %R0 be  
> dead
> or alive after running backwards over %R0<imp-kill,undef>?

There isn't a client for backward() right now so I am not too  
concerned about it. But in this case, it would assume R0 is not-live.

>
>
> Could we make it work with stricter semantics? Let "undef implies  
> dead"
> be the rule? Then this:
>
> %R0<def,undef> = IMPLICIT_DEF
>
> would essentially mean the same as this:
>
> %R0<def,dead> = IMPLICIT_DEF
>
> And this:
>
> %R0<def> = INSERT_SUBREG %R0<undef>, %R0L<kill>, 1
>
> would require %R0 to be dead. I think that could work, but there may  
> be
> issues in livevars that I don't understand.

I am very hesitant to change the semantics. That will break a lot of  
assumptions in the register allocator. I think the current approach of  
having subreg lowering pass insert the right instructions to model  
"kill" is enough. Is it not?

Evan

>
> Strict semantics makes it a lot easier to reason about register  
> liveness
> when transforming the code in LowerSubregs and other places.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits