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

[Adam Nemet via llvm-commits]

anemet added a comment.

In http://reviews.llvm.org/D10161#233173, @sbaranga wrote:

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


Great!  I understand that your actual goal to get the backedge count into a suitable form for analysis by using predicates.  The tricky part that that needs multiple things: the whole predicated SCEV infrastructure *plus* a good way to expose this otherwise implicit step to clients so that they have a way to reason about the cost of adding these predicates.

That's why if possible I'd like to get the first set in where the clients are in full control (based on potentially wrapping pointers for example) and then start bringing in the whole backedge count complexity.

That's said it's entirely possible that I am oversimplifying things.

Thanks,
Adam


================
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;
+}
----------------
sbaranga wrote:
> 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.
I see.  It still feels like a common multi-step pattern that clients can get wrong.  I wonder if we can make the API better here.  We can discuss this after you split this into multiple patches.

================
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);
----------------
sbaranga wrote:
> Caching expressions would make sense to me. We would have to recompute the expressions every time we commit to a new predicate though.
> 
> 
I don't follow.


http://reviews.llvm.org/D10161