[PATCH] D10161: [SCEV][LoopVectorize] Allow ScalarEvolution to make assumptions about overflows

[Adam Nemet via llvm-commits]

anemet added a comment.

Hi Silviu,

Sorry about the delay!  Also my comments below are somewhat disorganized because I am still trying to wrap my head around this large patch.  I *think* that the overall direction is good but I have a hard time seeing how the different parts connect at this point.  I also have some ideas below of how you could probably restructure this patch to help the review.

As a high-level comment, why doesn't the strided assumption become another type of assumption after these changes?

> Added a SCEVPredicate in ExitLimit, to allow getting a backedge count

>  guarded by a SCEV predicate and patched the ExitLimit logic to handle the

>  predicate. Added code to generate SCEV overflow predicates when

>  computing the loop count from an icmp condition (more code paths could also theoretically do this).


Why is this needed?  It's not explained.

As another high-level comment, I am also wondering if this 1000+ line patch can be split up to ease review.  The steps I am thinking:

1. Somehow generalize/refactor the stride-checks to make them more similar to the overflow assumptions
2. Add the minimal predicate set logic and drive it from LAA or somewhere so that you can add tests for simple cases.  Perhaps only make strided assumptions work with predicate sets first.
3. Add overflow assumptions
4. Add the guarded back-edge count extension along with analysis tests
5. Add the ExitLimit mods with some more tests.  This may have to come before 4 but I don't really understand this change at this point.

This is just an quick idea, feel free to structure the patches differently but I would be more comfortable with a more incremental approach.

Thanks,
Adam


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:295-296
@@ -293,1 +294,4 @@
+
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet &Pred;
 };
----------------
Can you please make this a bit more precise?  E.g.

With these assumption satisfied the memory accesses become simple AddRecs which allows them to get analyzed.

Also it should either be called Preds or PredSet.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:556
@@ +555,3 @@
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet ScPredicates;
+
----------------
What's the deal with unique_ptr above and this here?  Can you explaining the ownership situation?

Also it's probably less confusing if we use the same variable name for PredicateSets.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:141-148
@@ +140,10 @@
+
+  OFAssumptionResult R = removeOverflowsWithAssumptions(Ret, L, SE);
+  R.P.add(&Preds);
+  // Only commit to the new predicates if we've succeeded.
+  if (R.Ret && !R.P.isNever()) {
+    Preds = R.P;
+    return R.Ret;
+  }
+  return Ret;
+}
----------------
Hmm, why don't you only write Preds after to ensure that we succeeded?   Isn't this the same as:

if (R.Ret && !R.P.isNever()) {
  R.P.add(&Preds);  <-- write Preds here.
  return R.Ret:
}

if not it needs a comment.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:159
@@ -128,3 +158,3 @@
   assert(AR && "Invalid addrec expression");
-  const SCEV *Ex = SE->getBackedgeTakenCount(Lp);
+  const SCEV *Ex = SE->getGuardedBackedgeTakenCount(Lp, Pred);
   const SCEV *ScEnd = AR->evaluateAtIteration(Ex, *SE);
----------------
We should not recompute this for every insert.  Also we need to bound the number of predicates so getting the guarded backedge count at a more central place is probably a better idea.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:568-572
@@ -533,6 +567,7 @@
 
-      if (hasComputableBounds(SE, StridesMap, Ptr) &&
-          // When we run after a failing dependency check we have to make sure
-          // we don't have wrapping pointers.
-          (!ShouldCheckStride ||
-           isStridedPtr(SE, Ptr, TheLoop, StridesMap) == 1)) {
+      bool Bounded = hasComputableBounds(SE, StridesMap, Ptr, TheLoop, Pred);
+      if (!Bounded) {
+        convertSCEVToAddRec(SE, StridesMap, Ptr, nullptr, TheLoop, Pred);
+        Bounded = hasComputableBounds(SE, StridesMap, Ptr, TheLoop, Pred);
+      }
+
----------------
This does not look tight enough.

Can't we fail in hasComputable... for reasons other than the SCEV not being an AddRec?
Also the return value of convertSCEV... should be checked rather than just repeating hasComputable... blindly.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:312-314
@@ -311,1 +311,5 @@
 
+  // Adds code to check the overflow assumptions made by SCEV
+  std::pair<Instruction *, Instruction *>
+  addRuntimeOverflowChecks(Instruction *Loc);
+
----------------
There is no def for this function.


http://reviews.llvm.org/D10161