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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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);
----------------
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132571