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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers marked 5 inline comments as done.
nickdesaulniers added a comment.

Will update based on some of the outstanding feedback.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:44
 
+static bool isUsedOutsideOfDefiningBlock(const CallBrInst *CBR) {
+  return llvm::any_of(CBR->getIndirectDests(), [](BasicBlock *Dest) {
----------------
jyknight wrote:
> efriedma wrote:
> > 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.
> It feels like an improvement.
> 
> Also fine with me to commit separately, if you'd like to.
https://reviews.llvm.org/D142940 (marking thread as done, please reopen if there's more todo here)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2127
   if (VMI != FuncInfo.ValueMap.end()) {
-    assert(!V->use_empty() && "Unused value assigned virtual registers!");
+    assert((!V->use_empty() || isa<CallBrInst>(V)) &&
+           "Unused value assigned virtual registers!");
----------------
jyknight wrote:
> This change could also be reverted in that case I think, because it'd no longer appear spuriously unused.
https://reviews.llvm.org/D142940 (marking thread as done, please reopen if there's more todo here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140160