[PATCH] D18878: [Polly] Allow overflow of indices with constant dimensions size

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Michael,

very nice change. I just have some minor comments.

Best,
Tobias


================
Comment at: lib/Analysis/ScopInfo.cpp:332
@@ -331,1 +331,3 @@
 
+  // For dimensions that have constant length, modulo the index by the length
+  // and add up the carry to the next higher dimension. This is how overflow is
----------------
constant length -> constant size

================
Comment at: lib/Analysis/ScopInfo.cpp:338
@@ +337,3 @@
+  // We currently do this only if we added at least one dimension, which means
+  // some dimension's indicies have not been specified, an indicator that some
+  // index values have been added together.
----------------
indicies -> indices

================
Comment at: lib/Analysis/ScopInfo.cpp:343
@@ +342,3 @@
+  auto *LArraySpace = isl_local_space_from_space(isl_space_copy(ArraySpace));
+  if (DimsMissing)
+    for (int i = DimsArray - 1; i > 0; i--) {
----------------
Meinersbur wrote:
> Removing this causes a lot more changes in unit tests.
This is a large and mostly independent piece of code. It would probably make sense to put this into its own function.

================
Comment at: test/ScopInfo/multidim_fixedsize_multi_offset.ll:28
@@ -15,6 +27,3 @@
 ;
-; This test case makes sure we do not miss parts of the array subscript
-; functions, which we previously did by only considering the first of a chain
-; of GEP instructions. Today, we detect this situation and do not delinearize
-; the relevant memory access. In the future, we may want to detect this
-; pattern and combine multiple GEP functions together.
+; Verify that the additional offset to A by accessing it though C is taken into
+; account.
----------------
through

================
Comment at: test/ScopInfo/process_added_dimensions.ll:3-4
@@ -2,9 +2,4 @@
 
-; This test case produces the following memory access which we try hoist:
-; { Stmt_for_cond40_preheader_5[i0] -> MemRef_call[0, 0, 2240] }. However, it
-; accesses an array of size "i32 MemRef_call[*][6][64]".  That is why we
-; should turn the whole SCoP into an invalid SCoP using corresponding bounds
-; checks. Otherwise, we derive the incorrect access.
-
-; CHECK: Valid Region for Scop: for.cond40.preheader.4 => for.end76
-; CHECK-NOT:     Region: %for.cond40.preheader.4---%for.end76
+; CHECK:      Invariant Accesses: {
+; CHECK-NEXT: }
+; CHECK:      Statements {
----------------
Meinersbur wrote:
> The unit test is accepted and I don't find anything wrong. Can somebody double-check?
> 
> Especially because there is no hoisted load, but MemRef_call[5, 5, 0] is the correct access (instead of MemRef_call[0, 0, 2240])
Looks good to me.


http://reviews.llvm.org/D18878