[PATCH] D140160: [llvm][SelectionDAGBuilder] codegen callbr.landingpad intrinsic

Thu Jan 26 13:44:21 PST 2023

nickdesaulniers added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:44

+static bool isUsedOutsideOfDefiningBlock(const CallBrInst *CBR) {
+  return llvm::any_of(CBR->getIndirectDests(), [](BasicBlock *Dest) {
----------------
efriedma wrote:
> nickdesaulniers wrote:
> > jyknight wrote:
> > > It's unfortunate to need to special case this. Perhaps the intrinsic should take the callbr's output value as an input -- instead of magically "poofing" its value into existence by looking at the terminating instruction of the predecessor block? Then this code could be deleted, I believe.
> > I designed the intrinsic this way because of this comment by @efriedma : https://reviews.llvm.org/D139565#inline-1347398
> > 
> > >> Having the callbr as an operand seems redundant. We can already look up the corresponding callbr by just grabbing the terminator of the predecessor.
> > 
> > I might still have a prototype in a local branch that does have the explicit operand and uses an intrinsic rather than instruction, so it's probably not too bad to resurrect that code.  I don't feel very strongly either way.
> > 
> > But I would like @efriedma to review @jyknight 's point above, since rewriting the tests is exhausting, and I would need to revise this patch and the two prior. @efriedma thoughts?
> At that point, I was mostly concerned about consistency with other IR constructs like landingpad. 
>  Given the current architecture (where it isn't really exposed to IR optimizations), I'm less concerned about that.  So if adding the operand simplifies the code overall, that's fine.
> 
> I haven't thought through whether there might be other complications to changing the intrinsic that way
OK, here's the diff. It wasn't too bad, though rebasing the diffs for specific patches will be fun.
https://gist.github.com/nickdesaulniers/6df8e1a15f23636e2bf8abec96d425c7

@efriedma @jyknight PTAL and let me know your thoughts. I'll wait for your feedback before rebasing that down into the correct patches.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140160