[PATCH] D139565: [IR] add new callbrpad instruction

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the feedback.  It sounds like the approach I outlined isn't too unreasonable?

In D139565#3982756 <https://reviews.llvm.org/D139565#3982756>, @efriedma wrote:

> In D139565#3980574 <https://reviews.llvm.org/D139565#3980574>, @nickdesaulniers wrote:
>
>> 2. Create a new function pass (as part of `TargetPassConfig::addISelPasses` so it runs before isel on LLVM IR) that:
>
> If it isn't practical to lower correctly in SelectionDAG without mutating the IR, that's fine.  I suspect GlobalISel doesn't want an IR pass, though; it's probably more straightforward there to lower callbr directly to a G_CALLBR instruction, then do the lowering directly on MIR .  So making it part of SelectionDAG isel might be better?  I guess GlobalISel can just ignore the intrinsic if it wants, though, so maybe a separate pass is fine.  (We can add a flag to dump the IR after the transform if you're worried about testing.)
>
> If you do make it a separate pass, you probably want it pretty close to isel so other transforms don't get confused.

GlobalISel cannot lower callbr today. See `IRTranslator::translateCallBr`.  Is GlobalISel "driven" by SelectionDAGISel, (like SelectionDAG is?)  (Sorry, my understanding of the instruction selection frameworks in LLVM is...nascent and partial).

I think perhaps it makes sense to  create transform machinery distinct from a pass.  That way we can start with a distinct IR pass prior to SelectionDAG (as part of the `TargetPassConfig::addISelPasses` group) which uses said transform machinery and is testable, then transition/reuse it from elsewhere if necessary if/when we implement `IRTranslator::translateCallBr` in GlobalISEL.

>> 3. Replaces the uses dominated by the indirect edge to use the newly created value.  We might need to insert phis where previously there were none (perhaps the SSA construction @efriedma refers to?).
>
> Right, this is what I was referring to when I mentioned SSA construction.  (You'd probably use the llvm::SSAUpdater utility on IR.)

oh, thank god that exists and thanks for pointing me to it! I was losing sleep last night thinking about how difficult this would be for me to do...that class looks very very helpful for this problem.

>> 3. Add a mapping from CallBrInst to either INLINEASM_BR MachineInstr or first defined virtreg (not sure yet), probably in SelectionDAGBuilder's FunctionLoweringInfo instance member. Create an entry in this mapping when lowering a callbr or inlineasm.  Consume this mapping info when visiting the "new value" created from 2.A.b above to insert COPYs as necessary.
>
> Not sure you're guaranteed to visit the callbr before its successors?

I'm not certain either; it looks like `SelectionDAGISel::SelectAllBasicBlocks` does a `ReversePostOrderTraversal` of the basic blocks of a function.   I think that means we visit the callbr before its successors, but I'm not sure what that means when we have a callbr loop back on itself (such that the basic block is both its own successor and predecessor, maybe edge splitting can help).

>> b. inserts a new SSA value produced from either a new instruction, intrinsic call, or perhaps even just a single entry phi.

Any thoughts on what this "token" should be? i.e. a new instruction, call to a new intrinsic, one entry phi with metadata, <other>?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139565