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

In http://reviews.llvm.org/D14296#286975, @sbaranga wrote:

> In http://reviews.llvm.org/D14296#286968, @jmolloy wrote:
>
> > And there are places in LoopVectorize that sem to use "PSE.getSE()" quite a bit... are we sure they aren't subverting the predicates? If they do, is that a correctness or a performance issue?
>
>
> It would only be a correctness problem if you would expect some expression to have some form and only the predicated version would have that problem. 
>  Otherwise SE.getSCEV() and PredSE.getSCEV() would produce expressions evaluating to the same values (as long as we pass the runtime checks).
>  I've audited all the uses in LV and I believe there wouldn't be any correctness problem. There might be issues in other passes (LLE in particular), which I plan to address in a future commit.


You guys lost me at this point.  AIUI, SE->getSCEV(value) returns the SCEV without any assumptions.  PSE->getSCEV(value) returns a SCEV with the assumption backed in.  The only problem I can see is when the latter is used without run-time guarding it with the corresponding predicate.   What am I missing, how is LLE potentially broken?


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:388
@@ +387,3 @@
+///     an AddRec expression for a certain value through expression rewriting,
+///     we will continue to get an AddRec expresion for that Value.
+///   - lowers the number of expression rewrites.
----------------
expression

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:406-409
@@ +405,6 @@
+private:
+  void updateGeneration();
+
+  typedef std::pair<unsigned, const SCEV *> RewriteEntry;
+  DenseMap<const SCEV *, RewriteEntry> RewriteMap;
+  ScalarEvolution &SE;
----------------
Please comment these too


http://reviews.llvm.org/D14296