[InlineSpiller] Only examine defs in BUNDLE instruction

[uabelho]

Summary

If we have a bundle like

BUNDLE %vreg0<imp-def,dead>
  * %vreg0<def> = <some_instr>

and the entire %vreg0 live range needs to be spilled to the stack, we still
shouldn't insert a spill of %vreg0 after the above bundle since that
particular def is dead and the verifier will then complain about a use
after the dead def.

To solve this problem when there seems to be a mismatch between the dead
flags in the BUNDLE instruction and the bundled instructions, we only
examine the defs of the BUNDLE instruction if it exists.

On some targets there are bundles without BUNDLE instructions, and in those
cases we continue to examine the def of each individual bundled
instruction.

[MatzeB]

This adds bundle-specific behaviour into the register allocator. I think this is wrong!

From an RAs perspective the bundle really should be the union of all operands on a single instruction. So the real question to solve here is what to do with an instruction that has "%vreg0<imp-def,dead>" and "%vreg0<def>" at the same time and not checking for bundles and changing behaviour when we have them.

[qcolombet]

lib/CodeGen/InlineSpiller.cpp

993

I think we need to converge on what a dead flag of something already defined and used mean before moving forward with a fix.

[uabelho]

Thanks for the comments!

I have no idea if this is the right fix to do, that's why I asked in the thread on llvm-dev if there was any
point in putting up a patch with this change or not.

We've run with this locally on our out-of-tree target for a while now and haven't seen anything break
from it, but if there is a better change then I'm all for it. I don't know what it is though.

lib/CodeGen/InlineSpiller.cpp

for (MachineRegisterInfo::reg_bundle_iterator
     bool hasLiveDef = false;
     for (const auto &OpPair : Ops) {
       MachineOperand &MO = OpPair.first->getOperand(OpPair.second);
       MO.setReg(NewVReg);
       if (MO.isUse()) {
         if (!OpPair.first->isRegTiedToDefOperand(OpPair.second))
           MO.setIsKill();
       } else {
+        // For bundled instructions: Only examine defs in the BUNDLE
+        // instruction itself if it exists.
+        MachineInstr *DefMI = OpPair.first;
+        if (DefMI->isBundled() &&
+            getBundleStart(DefMI->getIterator())->isBundle() &&
+            !DefMI->isBundle())
+            continue;
+
         if (!MO.isDead())
           hasLiveDef = true;
       }
     }
     DEBUG(dbgs() << "\trewrite: " << Idx << '\t' << *MI << '\n');
 
     // FIXME: Use a second vreg if instruction has no tied ops.
     if (RI.Writes)

test/CodeGen/MIR/AArch64/spill-dead-bundle.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=greedy -verify-regalloc -o - %s | FileCheck %s
+---
+name:            test
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr64 }
+  - { id: 1, class: gpr64 }
+liveins:
+  - { reg: '%x0' }
+body:             |
+  bb.0:
+    liveins: %x0
+
+    %0 = COPY %x0
+
+    INLINEASM $nop, 1, 12, implicit-def dead %x0, 12, implicit-def dead %x1, 12, implicit-def dead %x2, 12, implicit-def dead %x3, 12, implicit-def dead %x4, 12, implicit-def dead %x5, 12, implicit-def dead %x6, 12, implicit-def dead %x7, 12, implicit-def dead %x8, 12, implicit-def dead %x9, 12, implicit-def dead %x10, 12, implicit-def dead %x11, 12, implicit-def dead %x12, 12, implicit-def dead %x13, 12, implicit-def dead %x14, 12, implicit-def dead %x15, 12, implicit-def dead %x16, 12, implicit-def dead %x17, 12, implicit-def dead %x18, 12, implicit-def dead %x19, 12, implicit-def dead %x20, 12, implicit-def dead %x21, 12, implicit-def dead %x22, 12, implicit-def dead %x23, 12, implicit-def dead %x24, 12, implicit-def dead %x25, 12, implicit-def dead %x26, 12, implicit-def dead %x27, 12, implicit-def dead %x28, 12, implicit-def dead %fp, 12, implicit-def dead %lr, 12, implicit-def %sp
+
+    BUNDLE implicit-def dead %0, implicit-def %1, implicit %0 {
+      %0.sub_32 = COPY undef %wzr
+      %1 = COPY %0
+    }
+...
+
+# Force spill of %0 by destroying all registers with the nop.
+
+# The def of %0.sub_32 has no use outside of the BUNDLE and the implicit def of
+# %0 is marked dead, so there should be no spill of %0 after the BUNDLE.
+
+# CHECK:      STRXui %x0, %stack.0
+# CHECK-NEXT: INLINEASM $nop
+# CHECK-NEXT: %2 = LDRXui %stack.0
+# CHECK-NEXT: BUNDLE implicit-def dead %2, implicit-def dead %1, implicit %2 {
+# CHECK-NEXT:   %2.sub_32 = COPY undef %wzr
+# CHECK-NEXT:   %1 = COPY %2
+# CHECK-NEXT: }
+# CHECK-NOT:  STRXui