[InlineSpiller] add assert about spills post terminators

[nickdesaulniers]

Summary

This invariant is being violated in the test case
https://reviews.llvm.org/D77849, related to the use of the relatively
new ability for callbr to have return values, and MachineBasicBlocks
with INLINEASM_BR terminators to emit live out register defs.

As noted in the comment, this triggers invariant violations in
MachineVerifier via llc -verify-machineinstrs or
llc -verify-regalloc, since only MachineInstrs that are terminators
are allowed to follow the first terminator.

https://reviews.llvm.org/D75098 may rework this very assertion if we're
spilling via a (proposed) TCOPY MachineInstr.

[nickdesaulniers]

I'm not particularly attached to this patch; just posting it for now. None of the current test cases in tree violate it, only the test case from https://reviews.llvm.org/D77849 does.

[efriedma]

LGTM with a minor nit

llvm/lib/CodeGen/InlineSpiller.cpp

949

Probably don't need the "This is commonly observed with" bit.

[nickdesaulniers]

llvm/lib/CodeGen/InlineSpiller.cpp

949

Sure thing, I'll clean this up later today. And again, I'm not attached to this, so if we want to rip it out or change it for TCOPY, let's do it.

I want to first post my patch for an additional assert in branch folder. What I have I don't think will be controversial. One thing I noticed though is that MachineBasicBlock#terminators() is not tracking INLINEASM_BR's correctly, so I'll need to go fix that first, in order to post that assertion patch, to revisit this. Otherwise this assert will mask that assert.

[nickdesaulniers]

llvm/lib/CodeGen/InlineSpiller.cpp

949

Ah, it's because of https://reviews.llvm.org/D77849. MachineBasicBlock#getFirstTerminator() iterates backwards through the MachineBasicBlock's MachineInstrs to the first non-Terminal, then moves forward one and returns that.

The case in https://reviews.llvm.org/D77849 means MachineBasicBlock#getFirstTerminator() and thus MachineBasicBlock#terminators() are broken due to the unexpected spill location.

I can make by check more robust in my yet to be published patch by iterating each MachineInstr in a MachineBasicBlock, rather than just the termintors().

[nickdesaulniers]

• drop comment about llc flags, as per @efriedma

llvm/lib/CodeGen/InlineSpiller.cpp

static bool isRealSpill(const MachineInstr &Def) {
   // fully defined by Def. Otherwise, some of the lanes may not be
   // undef and the value of the VReg matters.
   return Def.getOperand(0).getSubReg();
 }
 
 /// insertSpill - Insert a spill of NewVReg after MI.
 void InlineSpiller::insertSpill(unsigned NewVReg, bool isKill,
                                  MachineBasicBlock::iterator MI) {
+  // Spill are not terminators, so inserting spills after terminators will
+  // violate invariants in MachineVerifier.
+  assert(!MI->isTerminator() && "Inserting a spill after a terminator");
   MachineBasicBlock &MBB = *MI->getParent();
 
   MachineInstrSpan MIS(MI, &MBB);
   MachineBasicBlock::iterator SpillBefore = std::next(MI);
   bool IsRealSpill = isRealSpill(*MI);
   if (IsRealSpill)
     TII.storeRegToStackSlot(MBB, SpillBefore, NewVReg, isKill, StackSlot,
                             MRI.getRegClass(NewVReg), &TRI);