[PATCH] D17247: [Polly] Track assumptions and restrictions separately

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for this patch. It looks very nice. I just have two semantical issues, I need feedback on and two smaller stylistic comments.


================
Comment at: include/polly/ScopInfo.h:1313
@@ -1312,7 +1312,3 @@
   /// constraints over the parameters we assume to be true. However, the
-  /// boundary context is less useful for dependence analysis and
-  /// simplification purposes as it contains only constraints that affect the
-  /// boundaries of the parameter ranges. As these constraints can become quite
-  /// complex, the boundary context and the assumed context are separated as a
-  /// meassure to save compile time.
-  isl_set *BoundaryContext;
+  /// invalid context ... TODO
+  isl_set *InvalidContext;
----------------
Comment is incomplete

================
Comment at: lib/Analysis/ScopInfo.cpp:591
@@ -590,2 +590,3 @@
   Outside = isl_set_remove_divs(Outside);
   Outside = isl_set_complement(Outside);
+  auto &Loc = getAccessInstruction() ? getAccessInstruction()->getDebugLoc()
----------------
Is there a specific reason we do not drop the complement and use the other assumption context?

================
Comment at: lib/Analysis/ScopInfo.cpp:594
@@ -595,1 +593,3 @@
+                                     : DebugLoc();
+  Statement->getParent()->addAssumption(INBOUNDS, Outside, Loc, true);
   isl_space_free(Space);
----------------
Plain boolean parameters make it difficult to understand the code without knowing the name of parameter 4. Maybe just introduce an enum that clearly describes the two cases? Maybe ASSUME_VALID, ASSUME_INVALID?

================
Comment at: lib/Analysis/ScopInfo.cpp:1312
@@ -1311,3 +1311,3 @@
     isl_set *InBoundIfExecuted =
-        isl_set_union(isl_set_complement(Executed), InBound);
 
----------------
Nice.

================
Comment at: lib/Analysis/ScopInfo.cpp:1748
@@ -1747,3 +1725,1 @@
-  BoundaryContext = isl_set_gist_params(BoundaryContext, getContext());
-  trackAssumption(WRAPPING, BoundaryContext, DebugLoc());
 }
----------------
Nice.

================
Comment at: lib/Analysis/ScopInfo.cpp:1913
@@ -1935,3 +1912,3 @@
   AssumedContext = simplifyAssumptionContext(AssumedContext, *this);
-  BoundaryContext = simplifyAssumptionContext(BoundaryContext, *this);
+  InvalidContext = simplifyAssumptionContext(InvalidContext, *this);
 }
----------------
Did you check that the very same simplication logic also works for invalid context.

Assuming you have code

```
   for (i = 0; i < N; i++)
S:     ...
```

here we have N > 0 when we project the domain on the parameter set.

Now, assuming the code is invalid for N > 1000. If we gist_simplify the invalid set, we loose this constraint, no?




================
Comment at: lib/Analysis/ScopInfo.cpp:2406
@@ -2405,3 +2382,1 @@
       isl_set *DomPar = isl_set_params(isl_set_copy(Domain));
-      addAssumption(ERRORBLOCK, isl_set_complement(DomPar),
-                    BB->getTerminator()->getDebugLoc());
----------------
Nice to drop the complement.

================
Comment at: lib/Analysis/ScopInfo.cpp:2507
@@ -2506,3 +2483,1 @@
   isl_set *UnboundedCtx = isl_set_params(Parts.first);
-  isl_set *BoundedCtx = isl_set_complement(UnboundedCtx);
-  addAssumption(INFINITELOOP, BoundedCtx,
----------------
Nice to drop the complement.

================
Comment at: lib/Analysis/ScopInfo.cpp:3093
@@ +3092,3 @@
+  if (!PositiveContext)
+    return false;
+
----------------
This should not be NULL, no?

================
Comment at: lib/Analysis/ScopInfo.cpp:3103
@@ +3102,3 @@
+  if (!NegativeContext)
+    return false;
+
----------------
This should not be NULL, no?

================
Comment at: lib/CodeGen/IslAst.cpp:337
@@ +336,3 @@
+  auto *PosCond = isl_ast_build_expr_from_set(Build, S->getAssumedContext());
+  auto *NegCond = isl_ast_build_expr_from_set(Build, S->getInvalidContext());
+  auto *NotNegCond = isl_ast_expr_sub(isl_ast_expr_from_val(OneV), NegCond);
----------------
Can we only add the condition of the invalid context if it is non-trivial? This will make the output nicer and also should reduce the number of test cases changed?

================
Comment at: test/Isl/CodeGen/exprModDiv.ll:42
@@ -43,1 +41,3 @@
+; CHECK:  %[[r2:[0-9]*]] = sub nsw i64 %p, %[[r1]]
+; CHECK:  %polly.access.A8 = getelementptr float, float* %A, i64 %[[r2]]
 
----------------
This change seems to be independently beneficial. Maybe it could be committed ahead of time?

================
Comment at: test/ScopInfo/assume_gep_bounds_2.ll:19
@@ -18,3 +18,3 @@
 ; CHECK:      Assumed Context:
-; CHECK-NEXT: [n, m, p] -> {  : p <= 20 and (n <= 0 or (n > 0 and m <= 20)) }
+; CHECK-NEXT:   [n, m, p] -> { : p <= 20 and (n <= 0 or (n > 0 and m <= 20)) }
 
----------------
Unnecessary whitespace changes?

================
Comment at: test/ScopInfo/long-sequence-of-error-blocks-2.ll:10
@@ +9,3 @@
+; However, since we keep both the assumed as well as invalid context that
+; problem is solved.
+;
----------------
Nice!


http://reviews.llvm.org/D17247