Mikael Holmén
2014-Aug-22 12:17 UTC
[LLVMdev] Help with definition of subregisters; spill, rematerialization and implicit uses
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? (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()) || (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? Thanks, Mikael
Quentin Colombet
2014-Aug-22 23:43 UTC
[LLVMdev] Help with definition of subregisters; spill, rematerialization and implicit uses
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
Mikael Holmén
2014-Aug-25 10:33 UTC
[LLVMdev] Help with definition of subregisters; spill, rematerialization and implicit uses
Hi Quentin, On 08/23/14 01:43, Quentin Colombet wrote: > [...]> > 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.Excellent! Ok so now I did @@ -932,15 +932,28 @@ void InlineSpiller::reMaterializeAll() { // Try to remat before all uses of snippets. bool anyRemat = false; for (unsigned i = 0, e = RegsToSpill.size(); i != e; ++i) { unsigned Reg = RegsToSpill[i]; LiveInterval &LI = LIS.getInterval(Reg); - for (MachineRegisterInfo::use_bundle_nodbg_iterator - RI = MRI.use_bundle_nodbg_begin(Reg), E = MRI.use_bundle_nodbg_end(); - RI != E; ) { - MachineInstr *MI = &*(RI++); - anyRemat |= reMaterializeFor(LI, MI); + + for (MachineRegisterInfo::reg_bundle_iterator + RegI = MRI.reg_bundle_begin(Reg), E = MRI.reg_bundle_end(); + RegI != E; ) { + MachineInstr *MI = &*(RegI++); + + // Debug values are not allowed to affect codegen. + if (MI->isDebugValue()) { + continue; + } + + // Analyze instruction. + SmallVector<std::pair<MachineInstr*, unsigned>, 8> Ops; + MIBundleOperands::VirtRegInfo RI + MIBundleOperands(MI).analyzeVirtReg(Reg, &Ops); + + if (RI.Reads) + anyRemat |= reMaterializeFor(LI, MI); } } if (!anyRemat) return; So with this change InlineSpiller::reMaterializeAll is more similar to InlineSpiller::spillAroundUses, and now I'm not aware of any crashes anymore. :) My test case works, and I haven't seen any other tests suddenly failing. Is this a good change that should be done not only locally in my version? Thanks alot for your help! /Mikael> > I let you give it a try. > > Thanks, > -Quentin >> >> Thanks, >> Mikael > > >