[PATCH] D78234: [BranchFolding] assert when removing INLINEASM_BR indirect targets

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:171
+            if (Op.isBlockAddress() &&
+                Op.getBlockAddress()->getBasicBlock() == MBB->getBasicBlock())
+              assert(false &&
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > `Op.isBlockAddress()` check isn't sufficient to filter out all the relevant cases, I think.  It's possible to pass a blockaddress to an asm that isn't a valid destination according to the IR.
> That's right, and we don't have a great way of checking that one `MachineBasicBlock` is the indirect successor to another `MachineBasicBlock`'s `INLINEASM_BR`.
> 
> Looking at `MachineBasicBlock#isInlineAsmBrIndirectTarget` looks like what we might want, but the call to `MachineBasicBlock#transferInlineAsmBrIndirectTargets` in `ScheduleDAGSDNodes::EmitSchedule` looks kind of fishy to me.
> 
> I tried adding `MBB->isSuccessor(&Pred) &&` but this fails because the predecessor/successor list is messed up (from the commit description of https://reviews.llvm.org/D77849).
> 
> Backing up, regardless of the `BlockAddress` being an indirect successor of the `INLINEASM_BR` as opposed to just input, wouldn't removing the target `MachineBasicBlock` be bad either way?  Even if it was the operand of any instruction, and we remove the corresponding basic block, that sounds bad.
Well, suppose we have an indirectbr, but we prove it dead, and erase it.  Then we have a "dangling" blockaddress that can't really be used for anything; by itself, that shouldn't justify keeping the block alive.  (In other places, I think we replace it with a constant "-1".)

This is why, like I mentioned before, we should have a "successor" list on INLINEASM_BR that doesn't use blockaddress operands, so we can avoid confusion like this.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:173
+              assert(false &&
+                     "Removing MBB that's an indirect target of INLINEASM_BR");
+#endif
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > llvm_unreachable?
> > 
> > I'm a little concerned iterating over the entire function is going to be expensive, but we probably erase address-taken blocks pretty rarely.
> Not sure what you mean by `llvm_unreachable`? Was that a suggestion for a change to this patch, or a joke about this case being impossible?
> 
> Yes, it's relatively expensive.  But:
> 1. we only do this for assertion builds.
> 2. the initial check `MBB->hasAddressTaken()` is exceptional, and almost always false (except for the test case from D77849). Once D78166 lands (after this, is my plan), then this should always be false, modulo LLVM having more bugs, or regressions.
> 
> Which I think justifies having "expensive" assertions.  It's "expensive" in terms of work it has to do in an extremely exceptional case, but quite cheap relative to my time (and I think Linus Torvalds' time) debugging runtime issues of code generated from this exceptional case.
assert(false) is essentially equivalent to llvm_unreachable().

Yes, I think the cost here is okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78234