[PATCH] D11278: [IndVars] Make loop varying predicates loop invariant.

[Sanjoy Das]

sanjoy marked 4 inline comments as done.
sanjoy added a comment.

Sending in new patch shortly.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:593
@@ +592,3 @@
+    ///   Increasing is false -> Once LHS `Pred` X is false then it remains
+    ///     false for all future iterations
+    ///
----------------
reames wrote:
> You might want to note the ordering of the arguments to the comparison.  You implicitly do it in the naming, but not in the comment.
I have re-written this comment, please take a look.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:919
@@ -901,1 +918,3 @@
 
+    /// Can the predicate LHS `Pred` RHS be rewritten into a predicate that is
+    /// loop invariant?  If so, return true and set Invariant{Pred|LHS|RHS} to
----------------
reames wrote:
> I really can't tell from this comment what this function actually does.  
Cleaned up the comment, PTAL.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6619
@@ -6618,1 +6618,3 @@
 
+bool ScalarEvolution::isMonotonicPredicate(const SCEVAddRecExpr *LHS,
+                                           ICmpInst::Predicate Pred,
----------------
reames wrote:
> If I'm reading this right, a strictly decreasing predicate can be converted to a strictly increasing one by just swapping the direction of the predicate right?  That seems like it might form the basis for either s simplification in this code, it's caller, or possibly a useful assert.
I think that's a good idea.  I'll split out a `Impl` version of this function and do the assert you suggested.  The assertion won't work with the function as written because the case for `UGT` isn't symmetric -- I didn't fill in the other cases because an `nuw` unsigned decreasing variable does not make much sense in practice -- but there is no reason why I cannot add the missing cases.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6638
@@ +6637,3 @@
+    if (LHS->getNoWrapFlags(SCEV::FlagNSW)) {
+      if (isKnownNonNegative(LHS->getStepRecurrence(*this))) {
+        Increasing = true;
----------------
reames wrote:
> I don't think NonNegative is enough here.  Don't you need Positive?  i.e. what's would a zero step value mean?
A zero step value means the induction variable is essentially a loop invariant value.  We don't really depend on the predicate actually flipping from false to true (for increasing predicates), all we care about is that //if// the predicate changes then it only changes from false to true.

A zero step value in itself is not very useful, but there may be places where SCEV can prove `X >= 0` but not prove `X > 0`, so it is helpful to be maximally general.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6705
@@ +6704,3 @@
+    auto P = Increasing ? Pred : ICmpInst::getInversePredicate(Pred);
+    if (isLoopBackedgeGuardedByCond(L, P, LHS, RHS)) {
+      InvariantPred = Pred;
----------------
reames wrote:
> This utility function is unfamilar to me.  Does this imply that if the backedge executes, the condition must be true?  Does it say anything about when the backedge is not taken?  Might we have problems with two potential early exits and the wrong one being selected?  (Given the condition is just made invariant, I think this would be fine...)
> Does this imply that if the backedge executes, the condition must be true?

Yes.

> Does it say anything about when the backedge is not taken?

No.

> Might we have problems with two potential early exits and the wrong one being selected?

I don't see how -- the condition should evaluate to whatever it was evaluating to before.

================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:195
@@ +194,3 @@
+
+    if (S == InvariantLHS || X == InvariantLHS)
+      NewLHS =
----------------
reames wrote:
> This looks correct, but rather restricted.  Could we do something slightly more general?  This might be a good follow up patch.  
Yes.  I think there is scope to extend this part to emit some basic arithmetic, but I think there is value in checking this in as is, and then taking stock of what cases we're still missing.

================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:204
@@ +203,3 @@
+    for (Value *Incoming : cast<PHINode>(IVOperand)->incoming_values()) {
+      if (!NewLHS && SE->getSCEV(Incoming) == InvariantLHS)
+        NewLHS = Incoming;
----------------
reames wrote:
> Is getting a SCEV for each incoming value expensive?  I don't have a good mental cost model for this.  
> 
> I might organize this as two loops with a break for clarity.
> 
> Or alternatively, put a break in for when both NewLHS and RHS are set.  
> Is getting a SCEV for each incoming value expensive? I don't have a good mental cost model for this.

IIUC, internally, computing a SCEV for a PHINode or addition on a phinode also computes SCEV for all of its incoming values, so this should be just a hashtable lookup since we've already computed `S`.  Having said that, I should not be calling `getSCEV` twice for no good reason, so I'll fix that.

> Or alternatively, put a break in for when both NewLHS and RHS are set.

Done.

================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:212
@@ +211,3 @@
+      // We could not find an existing value to replace either LHS or RHS.
+      // Generating new instructions has subtler tradeoffs, so avoid doing that
+      // for now.
----------------
reames wrote:
> Some info about those subtle tradeoffs?
> 
> Looking through the code, won't NewRHS either be S or X by definition?
> 
> I think we should only need to be finding the start index?
> Some info about those subtle tradeoffs?

While I don't see making loop varying predicates loop invariant being a pessimization, it can be performance neutral in the worst case.  If it is performance neutral then emitting extra instructions outside the loop to compute the loop invariant check's operands may be a pessimization overall.

> Looking through the code, won't NewRHS either be S or X by definition?
>
> I think we should only need to be finding the start index?

I don't see how I can do that without coupling this bit of code with the implementation of `isLoopInvariantPredicate`.


http://reviews.llvm.org/D11278