[PATCH] [Unfinished] Use modulo semantic to generate non-wrap assumptions

[Tobias Grosser]

Hi Johannes,

this patch looks already great. I its overall structure and only have a couple of minor comments (see below).

We may also want to add a test case A[128 * p] to test the modulo path.

Best,
Tobias


================
Comment at: lib/Analysis/ScopInfo.cpp:94
@@ -91,2 +93,3 @@
 
-  SCEVAffinator(const ScopStmt *Stmt);
+  /// @brief Flag to inidicate that modulo semantic should be used.
+  const bool useModulo;
----------------
indicate

================
Comment at: lib/Analysis/ScopInfo.cpp:108
@@ -95,1 +107,3 @@
+                                        SCEV::NoWrapFlags Flags);
+
   __isl_give isl_pw_aff *visit(const SCEV *Expr);
----------------
Why do you call this function 'NonWrap'? Is the point of this function not to add the integer wrapping? Could a name like 'addIntegerWrapping()' be a better fit?

================
Comment at: lib/Analysis/ScopInfo.cpp:131
@@ -118,1 +130,3 @@
+__isl_give isl_pw_aff *SCEVAffinator::getPwAff(ScopStmt *Stmt, const SCEV *Scev,
+                                               bool useModulo) {
   Scop *S = Stmt->getParent();
----------------
UseModulo (start with uppercase)

================
Comment at: lib/Analysis/ScopInfo.cpp:143
@@ +142,3 @@
+                                                     const SCEV *Expr,
+                                                     SCEV::NoWrapFlags Flags) {
+  Expr->dump();
----------------
A comment explaining how we implement the modulo of signed types might be helpful.

res = ((res + 2^7) mod (2 ^ 8)) - 2^7

================
Comment at: lib/Analysis/ScopInfo.cpp:144
@@ +143,3 @@
+                                                     SCEV::NoWrapFlags Flags) {
+  Expr->dump();
+  if (Flags & SCEV::FlagNSW)
----------------
Leftover.

================
Comment at: lib/Analysis/ScopInfo.cpp:147
@@ +146,3 @@
+    return PWA;
+  errs() << " No nsw!\n";
+
----------------
Leftover.

================
Comment at: lib/Analysis/ScopInfo.cpp:268
@@ -221,1 +267,3 @@
+  assert(S->getRegion().contains(Expr->getLoop()) &&
+         "Scop does not contain the loop referenced in this AddRec");
 
----------------
This assert now fires for a couple of test cases, e.g. :
test/ScopInfo/NonAffine/non-affine-loop-condition-dependent-access_2.ll

================
Comment at: lib/Analysis/ScopInfo.cpp:281
@@ -232,5 +280,3 @@
 
-    // TODO: Do we need to check for NSW and NUW?
-    return isl_pw_aff_add(Start, isl_pw_aff_mul(Step, LPwAff));
-  }
+  isl_pw_aff *AddRecPWA = isl_pw_aff_add(Start, isl_pw_aff_mul(Step, LPwAff));
 
----------------
It seems you used this change remove the special case of isZero(). This special-case-removal seems partially unrelated. Maybe you want to commit this cleanup ahead of time (no review needed).

================
Comment at: test/ScopInfo/simple_loop_1.ll:18
@@ -17,3 +17,3 @@
   %i = phi i64 [ 0, %entry ], [ %i.inc, %bb ]
-  %scevgep = getelementptr i64, i64* %a, i64 %i
+  %scevgep = getelementptr inbounds i64, i64* %a, i64 %i
   store i64 %i, i64* %scevgep
----------------
Not necessary, but if you commit these changes separately (no review required), we can see that most of the time we have indeed sufficient nsw information.

================
Comment at: test/ScopInfo/wraping_signed_expr_1.ll:21
@@ +20,3 @@
+
+define void @wrap(i32* %A, i64 %N, i64 %p) {
+bb:
----------------
The C code uses chars, but here %N and %p are i64. Is this intended? I would actually prefer a version using 'char' to see bounds that are easier to understand.

================
Comment at: test/ScopInfo/wraping_signed_expr_1.ll:31
@@ +30,3 @@
+bb4:                                              ; preds = %bb2
+  %tmp5 = add nsw nuw i64 %indvars.iv, 1
+  %tmp6 = getelementptr i32, i32* %A, i64 %tmp5
----------------
Would it make sense to add a version that lacks the 'nsw' flag here?

================
Comment at: test/ScopInfo/wraping_signed_expr_1_long.ll:21
@@ +20,3 @@
+
+define void @wrap(i64* %A, i64 %N, i64 %p) {
+bb:
----------------
char <> i64 mismatch

================
Comment at: test/ScopInfo/wraping_signed_expr_5.ll:5
@@ +4,3 @@
+;
+; XFAIL: *
+;
----------------
Missing change.

http://reviews.llvm.org/D9099

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/