[PATCH] D11977: Use modulo semantic to generate non-integer-overflow assumptions

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

some first comments. I still need to understand the algorithm better, but probably just wait to read the writeup in the gitlab document.


================
Comment at: include/polly/ScopInfo.h:822
@@ +821,3 @@
+  /// complex, the boundary context and the assumed context are separated as a
+  /// meassure to save compile time.
+  isl_set *BoundaryContext;
----------------
measure

================
Comment at: include/polly/Support/SCEVAffinator.h:61
@@ -59,1 +60,3 @@
 
+  /// @brief Compute the context in which integer wrapping is happending.
+  ///
----------------
happening

================
Comment at: lib/Analysis/ScopInfo.cpp:1210
@@ +1209,3 @@
+  AssumptionContext = isl_set_gist_params(AssumptionContext,
+                                          isl_union_set_params(getDomains()));
+  AssumptionContext = isl_set_gist_params(AssumptionContext, getContext());
----------------
Is this valid? We are simplifying our assumption context here with the parameter constraints coming from the domain which was generated assuming the assumptions
hold. Is this not another <-> loophole? If it is not, maybe a comment would help that explains why this is correct.

================
Comment at: lib/Analysis/ScopInfo.cpp:1537
@@ +1536,3 @@
+  BoundaryContext = isl_set_complement(BoundaryContext);
+  BoundaryContext = isl_set_intersect_params(BoundaryContext, getContext());
+  simplifyContexts();
----------------
Maybe put this into a separate function with a comment what you are actually doing.

Also, why are you intersecting with getContext() the second time. This adds context constraints to the BoundaryContext which we know hold (and which will likely be dropped in simplifyContext().

================
Comment at: lib/Analysis/ScopInfo.cpp:1635
@@ +1634,3 @@
+  RTContext = isl_set_intersect(RTContext, getBoundaryContext());
+  RTContext = simplifyAssumptionContext(RTContext);
+  return RTContext;
----------------
This simplification is again fishy as we use the already simplified domain to simplify
the assumptions that were used to simplify the domain.

================
Comment at: lib/Support/SCEVAffinator.cpp:83
@@ +82,3 @@
+    PWAMod = isl_pw_aff_intersect_domain(PWAMod, ExprDomain);
+  }
+
----------------
It seems worth describing what you are actually computing here. Also, it is unclear to me why intersecting here with the domain is save. Are you relying on the domain not
being fully built?

================
Comment at: lib/Support/SCEVAffinator.cpp:85
@@ +84,3 @@
+
+  return isl_pw_aff_ne_set(PWA, PWAMod);
+}
----------------
It seems you only add modulo wrapping at the very end, but not right when we construct the expressions. This makes the impression you only add modulo wrapping at the outermost level. I think you already replied that the inner levels will also be in
the cache and consequently we will actually have all the wrappings. If this is the case,
we probably want to add a comment.


http://reviews.llvm.org/D11977