[PATCH] [Polly] Delinearize _all_ accesses to a multi-dimensional array

[Johannes Doerfert]

some minor comments.

================
Comment at: lib/Analysis/ScopDetection.cpp:369
@@ -368,3 +368,3 @@
     SmallVector<const SCEV *, 4> Terms;
-    for (PairInsnAddRec PIAF : Context.NonAffineAccesses[BasePointer])
-      PIAF.second->collectParametricTerms(*SE, Terms);
+    for (const auto &PIAF : Context.Accesses[BasePointer]) {
+      const SCEVAddRecExpr *AccessFunction =
----------------
What does PIAF mean?

================
Comment at: lib/Analysis/ScopDetection.cpp:388
@@ +387,3 @@
+
+    if (IsNonAffine)
+      continue;
----------------
Did I miss an assignment of IsNonAffine or is this check always false?

================
Comment at: lib/Analysis/ScopDetection.cpp:405
@@ -383,1 +404,3 @@
+    for (const auto &PIAF : Context.Accesses[BasePointer]) {
+      const SCEVAddRecExpr *AF = dyn_cast<SCEVAddRecExpr>(PIAF.second);
       const Instruction *Insn = PIAF.first;
----------------
Could you use a cast or an assert here to indicate that we "know" it is an AddRec?

================
Comment at: lib/Analysis/ScopDetection.cpp:412
@@ +411,3 @@
+                                   Shape->DelinearizedSizes);
+        if (!Acc->DelinearizedSubscripts.empty()) {
+          for (const SCEV *S : Acc->DelinearizedSubscripts)
----------------
The nested if statements make this harder to understand than it actually is.

Maybe we should just copy the isEmpty check, first if it is empty, then if it is not.

================
Comment at: lib/Analysis/ScopDetection.cpp:424
@@ +423,3 @@
+      if (IsNonAffine) {
+        if (!AllowNonAffine)
+          invalid<ReportNonAffineAccess>(Context, /*Assert=*/true, AF, Insn,
----------------
Above you used continue here, then reported the invalid access and checked for kepp going, I like that way (the one above) better. Maybe you could even put the two into a small function.

================
Comment at: lib/Analysis/ScopDetection.cpp:469
@@ +468,3 @@
+  if (PollyDelinearize) {
+    // FIXME: We do not yet handle inconsistent element sizes.
+    Context.ElementSize[BasePointer] = SE->getElementSize(&Inst);
----------------
Shouldn't we at least detect them and bail?

Do we have a test case for this?

================
Comment at: test/ScopDetectionDiagnostics/ReportMultipleNonAffineAccesses.ll:55
@@ +54,3 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Not needed.

================
Comment at: test/ScopInfo/multidim_single_and_multidim_array.ll:7
@@ +6,3 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Not needed.

================
Comment at: test/ScopInfo/multidim_single_and_multidim_array.ll:38
@@ +37,3 @@
+; DELIN:   MustWriteAccess :=
+; DELIN:      [n] -> { Stmt_for_i_2[i0] -> MemRef_X[-1 + n, i0] };
+
----------------
Nice test case.

http://reviews.llvm.org/D5329