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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D78234#1987870 <https://reviews.llvm.org/D78234#1987870>, @efriedma wrote:

> InlineAsmBrIndirectTargets seems like a bug waiting to happen: if we split a block for whatever reason (for example, lowering a cmov), I think the InlineAsmBrIndirectTargets will remain attached to the wrong block.


I agree, there was a certain asymmetry in API between LLVM IR and MIR that I couldn't quite put my finger on, but your comment helps highlight it for me.

For LLVM IR, `CallBrInst` tracks its default and indirect targets.  ie. the `Instruction` tracks these, not the `BasicBlock`, so manipulations to the `BasicBlock` can't lose this information.  You don't ask "is this `BasicBlock` an indirect target of this `BasicBlock`," you ask "is this `BasicBlock` an indirect target of this `CallBrInst`?"

For MIR, `MachineBasicBlock` tracks its default and indirect targets.  ie. the `MachineBasicBlock` tracks these, not the `MachineInstr`, so (as you point out) manipulations to the `MachineBasicBlock` could corrupt their default+indirect targets.  Currently, we can ask "is this `MachineBasicBlock` an indirect target of this `MachineBasicBlock`," but we *should* match the symmetry of the LLVM IR and change this to be able to ask "is this `MachineBasicBlock` an indirect target of the `MachineInstr`?"

Does that sound right?

One thing that also seems asymmetrical to me is the subclassing that occurs at the LLVM IR level (`CallBrInst` inherits from `CallBase` inherits from `Instruction`), but at the MIR level we just have `MachineInstr` with `OpCode`s to differentiate kinds of instructions.  It seems unfortunate to move these lists or sets of basic blocks to every `MachineInstr`, even if the vast majority are never `INLINEASM_BR`.  Should I just subclass the `MachineInstr`, or will that lead to other headaches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78234