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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:171
+            if (Op.isBlockAddress() &&
+                Op.getBlockAddress()->getBasicBlock() == MBB->getBasicBlock())
+              assert(false &&
----------------
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.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:173
+              assert(false &&
+                     "Removing MBB that's an indirect target of INLINEASM_BR");
+#endif
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78234