[ARM] ARMLoadStoreOpt::UpdateBaseRegUses should stop on def

[john.brawn]

Summary

When UpdateBaseRegUses sees an instruction that defines the base register it must stop, as the base register value it is updating is no longer live. Ideally we would already have seen the register be killed (which is already checked for), but the kill flags may be inaccurate and we have to account for this.

[MatzeB]

Nice catch! Kill flags are indeed optional nowadays so the additional check for definesRegister is necessary. Do you happen to have a llvm-lit testcase that exposes the bug?

[john.brawn]

In D10566#191185, @MatzeB wrote:

Nice catch! Kill flags are indeed optional nowadays so the additional check for definesRegister is necessary. Do you happen to have a llvm-lit testcase that exposes the bug?

I have a .ll file that exposes the bug but it's ~150 lines long and generates assembly that's ~450 lines long, with the incorrectly-generated code sequence in the middle which is rather infeasible to test. I haven't been able to reduce it to produce a sensible test case - it looks like it's a very specific combination of regalloc and post-regalloc optimization that produces the input to the load/store optimizer that triggers the bug.

[MatzeB]

It's probably tricky to get the compiler into a state where the kill flag is omitted. I guess until we have a way to serialize MI it is very hard to a proper testcase. So LGTM without testcase.

llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp

for (; MBBI != MBB.end(); ++MBBI) {
 
     if (InsertSub) {
       // An instruction above couldn't be updated, so insert a sub.
       AddDefaultT1CC(BuildMI(MBB, MBBI, dl, TII->get(ARM::tSUBi8), Base), true)
         .addReg(Base).addImm(WordOffset * 4).addImm(Pred).addReg(PredReg);
       return;
     }
 
-    if (MBBI->killsRegister(Base))
+    if (MBBI->killsRegister(Base) || MBBI->definesRegister(Base))
       // Register got killed. Stop updating.
       return;
   }
 
   // End of block was reached.
   if (MBB.succ_size() > 0) {
     // FIXME: Because of a bug, live registers are sometimes missing from
     // the successor blocks' live-in sets. This means we can't trust that