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

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

sbaranga added a comment.

Thanks! Some replies inline. I'll have a new version that addresses these issues out soon.

-Silviu


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:390
@@ +389,3 @@
+///   - lowers the number of expression rewrites.
+class PredicatedScalarEvolution {
+public:
----------------
anemet wrote:
> 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.
It could live in SE.h as well, it isn't obvious for me where it should live either. I added it here because it seemed to be solving a problem specific to LV/LAA then SCEV.

================
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;
+}
+
----------------
anemet wrote:
>  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.
> 
It looks like we can in fact rewrite this to do a single lookup.


http://reviews.llvm.org/D14296