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

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a comment.

Hi Adam,

In http://reviews.llvm.org/D10161#232197, @anemet wrote:

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


Thanks! Restructuring the patch sounds like a good idea.

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


That's a good idea, we should be able to do that. I don't see any problem with making a predicate for it in one of the patches.

> 

> 

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


The exit limit is what's being cached. That result is used to generate the (exact) backedge count, and other things. In order to keep caching mostly the same, we need to add the predicate to the ExitLimit. So this is all part of getting getBackedgeCount to work with predicates.

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


Yes, I've also felt the need to split this up. At least all the places where we're making assumptions should probably be reviewed separately. My plans were slightly different though (more focused towards being able to get the backedge count). I'll have to think about the exact details, but it will probably end up being similar to what you've listed above.

> 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:576
@@ +575,3 @@
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet ScPredicates;
+
----------------
This should be removed as it doesn't do anything. The unique_ptr above should contain the predicates, LoopAccessInfo ownes it and all other classes/structs have reference to it.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:141-148
@@ +140,10 @@
+
+  AssumptionResult R = SE->getAddRecWithRTChecks(Ret, L);
+  R.Pred.add(&Preds);
+  // Only commit to the new predicates if we've succeeded.
+  if (R.Res && !R.Pred.isAlwaysFalse()) {
+    Preds = R.Pred;
+    return R.Res;
+  }
+  return Ret;
+}
----------------
R.P.add(&Preds) adds Preds to R.P, so that wouldn't write Preds. We use the temporary because we can only be sure if we've succeeded or not after we see the final set of predicates.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:158-161
@@ -127,6 +157,6 @@
   const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(Sc);
   assert(AR && "Invalid addrec expression");
-  const SCEV *Ex = SE->getBackedgeTakenCount(Lp);
 
+  const SCEV *Ex = SE->getGuardedBackedgeTakenCount(Lp, Pred);
   const SCEV *ScStart = AR->getStart();
   const SCEV *ScEnd = AR->evaluateAtIteration(Ex, *SE);
----------------
Caching expressions would make sense to me. We would have to recompute the expressions every time we commit to a new predicate though.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:591-606
@@ -556,8 +590,18 @@
 
-      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);
+      }
+
+      bool ValidStride = true;
+      if (ShouldCheckStride) {
+        ValidStride =
+            (isStridedPtr(SE, Ptr, TheLoop, StridesMap, Pred, true) == 1);
+      }
+
+      // When we run after a failing dependency check we have to make sure
+      // we don't have wrapping pointers.
+      if (Bounded && ValidStride) {
         // The id of the dependence set.
         unsigned DepId;
----------------
Yes, you're correct. A better solution seems to only make the assumption after we've seen that hasComputableBounds.. now returns true.

This change probably needs a closer look.

================
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);
+
----------------
anemet wrote:
> There is no def for this function.
Must be an artefact from the rebase. It's not being used anywhere either, so it just needs to be removed.


http://reviews.llvm.org/D10161