[PATCH] D78166: [InlineSpiller] add assert about spills post terminators

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/InlineSpiller.cpp:949
+  // Spill are not terminators, so inserting spills after terminators will
+  // violate invariants in MachineVerifier. This is commonly observed with
+  // -verify-machineinstrs or even -verify-regalloc.
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > Probably don't need the "This is commonly observed with" bit.
> 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.
Ah, it's because of https://reviews.llvm.org/D77849.  `MachineBasicBlock#getFirstTerminator()` iterates backwards through the `MachineBasicBlock`'s `MachineInstr`s 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()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78166/new/

https://reviews.llvm.org/D78166