[PATCH] D132571: [RLEV] Pick a correct insert point when incoming instruction is itself a phi node

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1400
+          (isa<PHINode>(Inst) || isa<LandingPadInst>(Inst)) ?
+          &*Inst->getParent()->getFirstInsertionPt() : Inst;
+        RewritePhiSet.emplace_back(PN, i, ExitValue, InsertPt, HighCost);
----------------
nikic wrote:
> nikic wrote:
> > reames wrote:
> > > nikic wrote:
> > > > reames wrote:
> > > > > nikic wrote:
> > > > > > Any reason we don't just use `PN->getIncomingBlock(i)->getTerminator()` as the insertion point? That seems like a more obvious place to materialize an edge value.
> > > > > I'd been trying for a minimal fix.  Do you mind if we land this, and then do a follow up change as you suggest?  It'll cause significant test diffs which I think obscure the (rather narrow) impact of the fix.  
> > > > Sure
> > > Went to do this in a follow up, and realized it's more complicated than it sounds.
> > > 
> > > Since a loop can have multiple exiting blocks which share a single exit block, there is no single source for the exiting edge.  Instead, we'd have to either a) duplicate the expansion or b) find a common dominating point.  The current code effectively does the later, and leaves the decision on whether to duplicate in order to sink to later code.
> > > 
> > > (Remember: SCEV expander reuses expressions, so expanding the same expression at the same point multiple times only results in one expansion.)
> > > 
> > > It's not entirely clear to me we *should* do running RLEV when we can't sink, but it's definitely not a straight forward NFC any more.
> > > 
> > > Unless you feel strongly about this, I'm not going to follow up further.  
> > Oh, that makes sense. It's fine to leave it as is then.
> > 
> > What we should be able to do is require that the loop has dedicated exits (which is part of loop simplify, and as such a prerequisite for indvars anyway), in which case placing the expansion in the exit and replacing the lcssa phi node should be fine (right?)
> Eh, ignore my last comment, that's just wrong. Dedicated exits don't forbid having multiple incoming edges from the same loop.
I think this should work.  That would imply the multiple entry phi handling in this code is redundant and could be simplified away as well.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132571