[LLVMdev] Help with definition of subregisters; spill, rematerialization and implicit uses

[Quentin Colombet]

Hi Mikael,

On Aug 22, 2014, at 5:17 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:

> Hi Quentin,
> 
> On 08/19/14 18:58, Quentin Colombet wrote:
> [...]
>> It seems that you will have to debug further the *** Bad machine code: Instruction loads from dead spill slot *** before we can be of any help.
> 
> Yes, I've done some more digging. Sorry for the long mail...
> 
> I get:
> 
> Inline spilling aN40_0_7:%vreg1954 [5000r,5056r:0)[5056r,5348r:1) 0 at 5000r 1 at 5056r
> 
> At this point I have the following live ranges for vreg1954:
> 
> %vreg1954 [5000r,5056r:0)[5056r,5348r:1)  0 at 5000r 1 at 5056r
> 
> And vreg1954 is mentioned in these instructions:
> 
> 5000B		%vreg1954:hi16<def,read-undef> = mv_any16 32766
> [...]
> 5048B		mv_ar16_r16_rmod1 %vreg1954:hi16, %vreg1753
> 5056B		%vreg1954:lo16<def> = mv_nimm6_ar16 0
> 5064B		Store40FI %vreg1954, <fi#2>
> [...]
> 5128B		%vreg223<def> = COPY %vreg1954
> [...]
> 5216B		%vreg1178<def> = COPY %vreg1954
> [...]
> 5348B		%vreg1955<def> = COPY %vreg1954
> 
> Then it tries to rematerialize:
> 
> Value %vreg1954:0 at 5000r may remat from %vreg1954:hi16<def,read-undef> = mv_any16 32766
> 
> And it examines all use points of vreg1954 to see if it can remat or not:
> 
> 	cannot remat for 5128e	%vreg223<def> = COPY %vreg1954
> 	cannot remat for 5216e	%vreg1178<def> = COPY %vreg1954
> 	remat:  5044r	%vreg1956:hi16<def,read-undef> = mv_any16 32766
> 	        5048e	mv_ar16_r16_rmod1 %vreg1956:hi16<kill>, %vreg1753
> 
> 	cannot remat for 5064e	Store40FI %vreg1954, <fi#2>
> 	cannot remat for 5348e	%vreg1955<def> = COPY %vreg1954
> All defs dead: %vreg1954:hi16<def,read-undef,dead> = mv_any16 32766
> Remat created 1 dead defs.
> Deleting dead def 5000r	%vreg1954:hi16<def,read-undef,dead> = mv_any16 32766
> 1 registers to spill after remat.
> 
> What strikes me here is that it never mentions the instruction
> 
> 5056B		%vreg1954:lo16<def> = mv_nimm6_ar16 0
> 
> where we do have a read of vreg1954:hi16 since there is no read-undef on the def operand.
> 
> Is this how it's intended to be?

What does not seem intended to me :).

> 
> (The use points are found by
> 
>    for (MachineRegisterInfo::use_bundle_nodbg_iterator
>         RI = MRI.use_bundle_nodbg_begin(Reg), E = MRI.use_bundle_nodbg_end();
>         RI != E; ) {
>      anyRemat |= reMaterializeFor(LI, MI);
>    }
> 
> and
> 
> MachineRegisterInfo::defusechain_instr_iterator::advance
> 
> seems to skip all def operands for use_bundle_nodbg_iterator since ReturnDefs is false. So it will happily advance past the instruction setting lo16.)
> 
> Then after the remats it does
> 
> spillAroundUses %vreg1954
> 
> and there I get
> 
> 	reload:   5052r	%vreg1957<def> = Load40FI <fi#2>
> 	rewrite: 5056r	%vreg1957:lo16<def> = mv_nimm6_ar16 0
> 
> since no remat was inserted before 5056. But noone has stored anything at fi#2 so I end up with
> 
> *** Bad machine code: Instruction loads from dead spill slot ***
> 
> Does everything above, except the error at the end, look ok to you?
> 
> 
> I tried fiddling with MachineRegisterInfo::defusechain_instr_iterator to make it also return the
> 
> %vreg1954:lo16<def> = mv_nimm6_ar16 0
> 
> instruction, and then my program actually compiled succesfully!
> 
> My diff:
> 
> @@ -886,11 +886,11 @@ public:
>     explicit defusechain_instr_iterator(MachineOperand *op) : Op(op) {
>       // If the first node isn't one we're interested in, advance to one that
>       // we are interested in.
>       if (op) {
>         if ((!ReturnUses && op->isUse()) ||
> -            (!ReturnDefs && op->isDef()) ||
> +            (!ReturnDefs && op->isDef() && !op->readsReg()) ||

This fix is not correct because you will return a definition operand in place of our missing “implicit use”, which, I believe, will lead to funny crashes.

>             (SkipDebug && op->isDebug()))
>           advance();
>       }
>     }
>     friend class MachineRegisterInfo;
> @@ -907,11 +907,11 @@ public:
>           else
>             assert(!Op->isDebug() && "Can't have debug defs");
>         }
>       } else {
>         // If this is an operand we don't care about, skip it.
> -        while (Op && ((!ReturnDefs && Op->isDef()) ||
> +        while (Op && ((!ReturnDefs && Op->isDef() && !Op->readsReg()) ||
>                       (SkipDebug && Op->isDebug())))
>           Op = getNextOperandForReg(Op);
>       }
>     }
>   public:
> 
> 
> But of course then other tests fail. For example:
> 
> build-all/./bin/llc < test/CodeGen/R600/literals.ll -march=r600 -mcpu=redwood
> 
> gives
> 
> llc: ../lib/CodeGen/TwoAddressInstructionPass.cpp:684: void (anonymous namespace)::TwoAddressInstructionPass::scanUses(unsigned int): Assertion `SrcRegMap[NewReg] == Reg && "Can't map to two src registers!"' failed.
> 
> So I suppose there are assumptions that defusechain_instr_iterator ignores implicit sub register use when defining some sub register. :/
> 
> What's your thoughts on this?

Conceptually, we could add an implicit use of the high part on the definition of the low part.
That said, this will not solve the problem, because rematerializing wouldn’t change the vreg of the low part, and the range would become disjoint.
Assuming we solve that problem, I am not sure this will be understood correctly by the verifier. The verifier might expect that the related register is fully defined before an implicit-use. This may also over-constrained the scheduler… I do not know that part.

Alternative we could do the same thing as insertReload:
Walk all uses *and* defs and apply the rematerialization for each use or readsRegs.
Notice the different iterator for remat and for reload. They should be the same.

I let you give it a try.

Thanks,
-Quentin
> 
> Thanks,
> Mikael