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

[Renato Golin]

rengolin added a subscriber: rengolin.
rengolin added a comment.

Hi Silviu,

I have some comments, mostly nitpicks. I'll continue the review as I go, but here are the first ones.

cheers,
--renato


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:636
@@ -604,2 +635,3 @@
   /// replaceSymbolicStrideSCEV).  If there is no cached result available run
-  /// the analysis.
+  /// the analysis. If UseAssumptions is true, the user must add the
+  /// run-time checks collected by the AssumingScalarEvolution pass if
----------------
this comment seems a bit terse. UseAssumptions is an argument to some methods in SCEV, nothing to do with LoopInfo.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:189
@@ +188,3 @@
+    /// assumptions were made and nothing needs to be checked at run-time.
+    virtual bool isAlways() const = 0;
+    /// Return true if we consider this to be always false or if we've
----------------
nitpick: maybe isAlwaysTrue / isNeverTrue or isAlwaysTrue / isAlwaysFalse would be more descriptive?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:245
@@ +244,3 @@
+  private:
+    bool Never;
+    /// Storage for different predicates that make up this Predicate Set.
----------------
No description?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:246
@@ +245,3 @@
+    bool Never;
+    /// Storage for different predicates that make up this Predicate Set.
+    SmallVector<SCEVAddRecOverflowPredicate, 16> AddRecOverflows;
----------------
Why the duplication? Will you have one for each further type?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:273
@@ +272,3 @@
+    /// Methods for support type inquiry through isa, cast, and dyn_cast:
+    static inline bool classof(const SCEVPredicate *P) {
+      return P->getType() == pSet;
----------------
Why here, and not in SCEVPredicate?


http://reviews.llvm.org/D10161