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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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) {
----------------
nickdesaulniers wrote:
> 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.
Looks fine?  I mean, the simplification isn't really that big, but it doesn't really add any complexity elsewhere.

If it's going to be a pain to rebase, I'd be okay just committing it as a followup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140160