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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a subscriber: craig.topper.
nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:171
+            if (Op.isBlockAddress() &&
+                Op.getBlockAddress()->getBasicBlock() == MBB->getBasicBlock())
+              assert(false &&
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > 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.
> > > by itself, that shouldn't justify keeping the block alive.
> > 
> > This patch doesn't keep the `MachineBasicBlock` alive.  I also don't advocate for keeping the `MachineBasicBlock` alive.
> > 
> > When you fail this assertion, the solution is not "we should keep the `MachineBasicBlock` alive," it's "we should figure out why we proved a `MachineBasicBlock` dead when it clearly has a `BlockAddress` that references it, as that is weird and likely a bug."
> > 
> > Maybe I can reflect that sentiment via an additional comment added to this patch, so that future travelers that may experience this assertion (hopefully never) understand the intent and how best to proceed?
> Or a more precise error string in the `llvm_unreachable`?
In another branch, I tried deleting `MachineBasicBlock::transferInlineAsmBrIndirectTargets()`.  All tests in tree pass.  So I think I'll roll that change into this one, then I can use `MachineBasicBlock#isInlineAsmBrIndirectTarget` properly here.

Following up on the RFC discussion, I found @craig.topper 's comment (https://reviews.llvm.org/D53765?id=184024#inline-508610) on the RFC which had `setIsAsmGotoTarget` (which I suspect didn't persist when `INLINEASM_BR` was created), though was added as `addInlineAsmBrIndirectTarget` recently).

@void @efriedma any preference on me making that change in this patch vs in a separate patch, first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78234