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

[Jakob Stoklund Olesen]

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.

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


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.

Strict semantics makes it a lot easier to reason about register liveness
when transforming the code in LowerSubregs and other places.