This is an archive of the discontinued LLVM Phabricator instance.

[InlineSpiller] add assert about spills post terminators
ClosedPublic

Authored by nickdesaulniers on Apr 14 2020, 4:25 PM.

Details

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.

Diff Detail

Event Timeline

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 accepted this revision.Apr 14 2020, 5:29 PM

LGTM with a minor nit

llvm/lib/CodeGen/InlineSpiller.cpp
949

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

This revision is now accepted and ready to land.Apr 14 2020, 5:29 PM
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.

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 marked 2 inline comments as done.
  • drop comment about llc flags, as per @efriedma
nickdesaulniers marked an inline comment as done.Apr 15 2020, 12:57 PM
This revision was automatically updated to reflect the committed changes.