[PATCH] D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2)

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

In D57428#1430436 <https://reviews.llvm.org/D57428#1430436>, @sanjoy wrote:

> Apologies for the late review Warren.
>
> I agree that it is weird that the insert point is a PHI node.  Have you traced through the code to see how we're ending up in that situation?  Is IndVarSimplify choosing this insertion point?


Thanks for the comment, Sanjoy.

Loosely, yes, IndVarSimplify is choosing this insertion point.  To elaborate, it selects that insertion point in `WidenIV::createWideIV()`, in the following code:

  // The rewriter provides a value for the desired IV expression. This may
  // either find an existing phi or materialize a new one. Either way, we
  // expect a well-formed cyclic phi-with-increments. i.e. any operand not part
  // of the phi-SCC dominates the loop entry.
  Instruction *InsertPt = &L->getHeader()->front();
  WidePhi = cast<PHINode>(Rewriter.expandCodeFor(AddRec, WideType, InsertPt));

where the header Basic Block starts out with 2 PHI nodes.

(Side note: The `AddRec` here is:

  {0,+,(sext i32 (%0 /u %denominator) to i64)}<nsw><%while.body>

More on this point below.)

To me, it doesn't feel like changing the above code to use `getFirstInsertionPt()` on the parent BB of `InsertPt` is a good approach.  I just experimented with that now, and I found it solved the first instance of the insertion point being a PHI node, but as the above `AddRec` was recursively processed, it eventually called `SCEVExpander::getAddRecExprPHILiterally()`, which has the following code:

  // Expand the step somewhere that dominates the loop header.
  Value *StepV = expandCodeFor(Step, IntTy, &L->getHeader()->front());

That `&L->getHeader()->front()` arg to `expandCodeFor()` is passed to `setInsertPoint()`, and this again causes the same trouble.  If I change this to also get the `getFirstInsertionPt()` on the parent BB of `&L->getHeader()->front()`, then my test-case passes.

So tweaking these two points (`WidenIV::createWideIV()` and `SCEVExpander::getAddRecExprPHILiterally()`) is another possible solution to investigate.  But (a) I don't feel like these changes are safe (but I'm not experienced in this area), and (b) I wonder whether there are other places that would need similar changes but just aren't triggered by my test-case.

One further observation about the `AddRec`:

  {0,+,(sext i32 (%0 /u %denominator) to i64)}<nsw><%while.body>

All that is really being attempted here is to hoist the sign-extension of the quotient out of a loop.  The actual division is before the loop in the input .ll file (and it was before the loop in the original C++ code).  In other words, we aren't trying to hoist an unsafe division out of the loop because the division never was even //in //the loop.  But `SafeToHoist()` isn't aware of that -- it "sees" the `/u` in the expression, and decides it shouldn't hoist it.  If instead, we could somehow know that the only expression that we're considering hoisting is the `sext` (and then allow that expression to be hoisted), then all would be fine (FTR, the `sext` eventually does get hoisted by other optimizations).  I say this with the caveat that I don't deeply understand this SCEV mechanism.


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

https://reviews.llvm.org/D57428