[PATCH] D14296: [LV][LAA] Add a layer over SCEV to apply run-time checked knowledge on SCEV expressions

[Adam Nemet via llvm-commits]

anemet added a comment.

Pretty minor comments.  Sorry about the delay.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:504-506
@@ -506,5 +503,5 @@
 ///
 /// If pointers can wrap or can't be expressed as affine AddRec expressions by
 /// ScalarEvolution, we will generate run-time checks by emitting a
 /// SCEVUnionPredicate.
 ///
----------------
You probably want to say that union predicate is now inside PSE.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:390
@@ +389,3 @@
+///   - lowers the number of expression rewrites.
+class PredicatedScalarEvolution {
+public:
----------------
Why is this in LoopUtils.h, for example as opposed to SE.h?  I don't have a preference but it's not immediately obvious to me.

================
Comment at: lib/Transforms/Scalar/LoopDistribute.cpp:764
@@ -763,3 +763,3 @@
     // Don't distribute the loop if we need too many SCEV run-time checks.
-    const SCEVUnionPredicate &Pred = LAI.Preds;
+    const SCEVUnionPredicate &Pred = LAI.PSE.getPredicate();
     if (Pred.getComplexity() > DistributeSCEVCheckThreshold) {
----------------
Nit: We keep going back between plural and singular.  The point I think is trying to avoid somehow suggesting that we can only handle a single predicate by using singular.

Can we use getUnionPredicate or something like that perhaps?  What do you think?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:734-749
@@ +733,18 @@
+
+const SCEV *PredicatedScalarEvolution::getSCEV(Value *V) {
+  const SCEV *Expr = SE.getSCEV(V);
+  auto II = RewriteMap.find(Expr);
+  if (II == RewriteMap.end()) {
+    const SCEV *NewSCEV = SE.rewriteUsingPredicate(Expr, Preds);
+    RewriteMap[Expr] = {Generation, NewSCEV};
+    return NewSCEV;
+  }
+
+  if (Generation == II->second.first)
+    return II->second.second;
+
+  const SCEV *NewExpr = SE.rewriteUsingPredicate(II->second.second, Preds);
+  RewriteMap[Expr] = {Generation, NewExpr};
+  return NewExpr;
+}
+
----------------
 I think the logic would be tighter like this:
 
  if (found && version matches)
    return it
  
  // Use the previously rewritten value to preserve the order of applying 
  // the predicates
  if (found)
    Expr = II->second.second
  
  NewExpr = SE.rewriteUsingPredicate(Expr, Preds);
  RewriteMap[Expr] = {Generation, NewExpr};
  return NewExpr;


Also I forgot the API now but is there a way to get an iterator that we can always write (regardless whether we're adding a new entry).  You do two lookups here.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:763-765
@@ +762,5 @@
+void PredicatedScalarEvolution::updateGeneration() {
+  Generation++;
+  // The generation number wrapped. Recompute everything.
+  if (Generation == 0) {
+    for (auto &II : RewriteMap) {
----------------
Nit: I would fold the increment in the if condition.


http://reviews.llvm.org/D14296