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

Philip Reames listmail at philipreames.com
Fri Jul 17 18:47:38 PDT 2015


reames added inline comments.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:586
@@ +585,3 @@
+    ///
+    /// In other words, if isMonotonicPredicate returns true then for all
+    /// (loop-invariant) X then:
----------------
This part of the comment makes no sense to me.  Typos?

================
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
+    ///
----------------
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.

================
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
----------------
I really can't tell from this comment what this function actually does.  

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6619
@@ -6618,1 +6618,3 @@
 
+bool ScalarEvolution::isMonotonicPredicate(const SCEVAddRecExpr *LHS,
+                                           ICmpInst::Predicate Pred,
----------------
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.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6638
@@ +6637,3 @@
+    if (LHS->getNoWrapFlags(SCEV::FlagNSW)) {
+      if (isKnownNonNegative(LHS->getStepRecurrence(*this))) {
+        Increasing = true;
----------------
I don't think NonNegative is enough here.  Don't you need Positive?  i.e. what's would a zero step value mean?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6686
@@ +6685,3 @@
+  bool Increasing;
+  if (isMonotonicPredicate(ArLHS, Pred, Increasing)) {
+    // If the predicate "ArLHS `Pred` RHS" monotonically increases from false to
----------------
Early return preferred.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6704
@@ +6703,3 @@
+
+    auto P = Increasing ? Pred : ICmpInst::getInversePredicate(Pred);
+    if (isLoopBackedgeGuardedByCond(L, P, LHS, RHS)) {
----------------
I think I've convinced myself the reasoning here for an increasing predicate (false->true) is valid, but I'm struggling with the reasoning for the decreasing (true->false) case.  In particular, the order of operands in the result seems suspicious.  

... I convinced myself this is correct, but a better comment about why would help.  In particular, you're relying on the fact that inverting a decreasing predicate creates an increasing one which switches at the same point.  This is fine, but should be explained.

Also, is this robust against future enhancements to isMonotonicPredicate?  I think it is, but how do we ensure that?

One natural case to handle would be an equals comparison.  That's not monotonic, so I think we're fine...  that would take the form of a weird unswitch (i.e. start..end doesn't include test value).  What about the case where an equals comparison is monotonic precisely because it's controlling whether the induction variable is incremented?  Does that cause us problems?  We'd end with either an infinite loop or our weird "monotonic equal" being the guard on the backedge... I think we'd be fine.  The infinite loop case is caught by the isLoopBackedgeGuardByCond, the backedge guard itself is caught by... well nothing?

So yeah, we do need to make sure we define this in such a way that the backedge guard itself can't be considered monotonic.  Or at least include that test case to prevent future person from shooting themselves in the foot.  :)



================
Comment at: lib/Analysis/ScalarEvolution.cpp:6705
@@ +6704,3 @@
+    auto P = Increasing ? Pred : ICmpInst::getInversePredicate(Pred);
+    if (isLoopBackedgeGuardedByCond(L, P, LHS, RHS)) {
+      InvariantPred = Pred;
----------------
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...)

================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:195
@@ +194,3 @@
+
+    if (S == InvariantLHS || X == InvariantLHS)
+      NewLHS =
----------------
This looks correct, but rather restricted.  Could we do something slightly more general?  This might be a good follow up patch.  

================
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;
----------------
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.  

================
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.
----------------
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?


http://reviews.llvm.org/D11278







More information about the llvm-commits mailing list