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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

It's not clear to me an IR instruction is necessary.  I mean, I understand why you want a callbrpad-equivalent MachineInstr: the only existing infrastructure for jumping out of the middle of a block is the exception-handling infrastructure, so you want to handle callbr in a similar way.  I guess if you have a MachineInstr, but you don't have an IR instruction, doing the translation would involve SSA construction in isel, which is maybe a bit awkward.  But I'm not sure that's sufficient reason to introduce an IR instruction.  At the IR level, callbr is a terminator, and we produce the same value along all edges, so IR transforms don't benefit from distinguishing between direct and indirect edges.

If we do decide to go with an IR instruction, making the callbrpad optional seems likely to cause confusion in IR passes, in terms of what transformations are actually legal.  To make this formulation work, indirect destinations have to be distinct from the direct destination.  (Otherwise, it's not clear whether values produced directly are actually available in a given block.)  If callbrpad is mandatory, this falls out in an obvious way: the direct destination can't contain a callbrpad, and the indirect destinations must contain one, so they can't get folded together.  If it's optional, we need extra checks in a bunch of places to make sure the destinations don't get folded together.



================
Comment at: llvm/docs/LangRef.rst:12159
+
+The '``callbrpad``' instruction takes one argument, which must be a
+`callbr <i_callbr>`. The type of the '``callbrpad``' corresponds to the type of
----------------
Having the callbr as an operand seems redundant.  We can already look up the corresponding callbr by just grabbing the terminator of the predecessor.


================
Comment at: llvm/test/Verifier/callbr.ll:74
+
+; CHECK-NOT: Instruction does not dominate all uses!
+define i32 @test5(i1 %var) {
----------------
Using CHECK-NOT like this is really fragile.  I'd suggest having two tests: one that tests that valid IR isn't rejected, and one that checks that the expected diagnostics are produced for invalid IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139565