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

[Tobias Grosser]

Hi Johannes, hi Sebastian

thanks for the review. It was very valuable and nicely improved this patch. I will send an updated version right after.

================
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 =
----------------
jdoerfert wrote:
> What does PIAF mean?
Pair-Instruction-AliasFunction

Yes, this is definitely an abbreviation that is not well known. It was here before, but I can change it to just 'Pair'. That should be clear, as we extract the two elements in the following two lines.

================
Comment at: lib/Analysis/ScopDetection.cpp:388
@@ +387,3 @@
+
+    if (IsNonAffine)
+      continue;
----------------
jdoerfert wrote:
> Did I miss an assignment of IsNonAffine or is this check always false?
Good catch. In fact this part was just wrong. In case this is not a SCEVAddRecExpr, we should not report the full array as non-affine, but instead just do not collect parametric terms there. If I would just have fixed the code as written, every constant access e.g. A[0][0] would have broken the delinearization.

I added this case to the test case submitted later in this patch.

================
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;
----------------
jdoerfert wrote:
> Could you use a cast or an assert here to indicate that we "know" it is an AddRec?
Unfortunately we don't know. In case this is not a SCEVAddRecExpr, we now assume (and verify) the access corresponds to the innermost dimension.

================
Comment at: lib/Analysis/ScopDetection.cpp:412
@@ +411,3 @@
+                                   Shape->DelinearizedSizes);
+        if (!Acc->DelinearizedSubscripts.empty()) {
+          for (const SCEV *S : Acc->DelinearizedSubscripts)
----------------
jdoerfert wrote:
> 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.
I pulled out the reporting of a failed delinearization. This removed one level of indentation.

================
Comment at: lib/Analysis/ScopDetection.cpp:424
@@ +423,3 @@
+      if (IsNonAffine) {
+        if (!AllowNonAffine)
+          invalid<ReportNonAffineAccess>(Context, /*Assert=*/true, AF, Insn,
----------------
jdoerfert wrote:
> 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.
I had this before. The problem is that the code for this reporting would then be replicated a couple of times. I also thought about using a separate function, but as the result of the reporting can have control flow implications, I can not make this a simple function call, but would need something like the following for every case where we assign IsNonAffine:

```
if (reportAndCheckForReturn(Inst, S, &GlobalNonAffine)
  return false;
```

The other changes allowed a couple of simplifications. Maybe it is now already simple enough.

================
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);
----------------
jdoerfert wrote:
> Shouldn't we at least detect them and bail?
> 
> Do we have a test case for this?
This is somehow unrelated, but you are right it should be fixed. I added a separate patch that fixes this issue before we perform the actual all-accesses patch.

================
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"
+
----------------
jdoerfert wrote:
> Not needed.
Removed data layout and triple.

================
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"
+
----------------
jdoerfert wrote:
> Not needed.
Removed the triple, the data-layout is required for -polly-scops.

http://reviews.llvm.org/D5329