[PATCH] D125805: [LoopInterchange] Support multi-level reduction loops

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Consider being less specific to only recognize nested loops, but PHINodes in the loop body in general.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:789
         Value *V = followLCSSA(PHI.getIncomingValueForBlock(L->getLoopLatch()));
-        PHINode *InnerRedPhi = findInnerReductionPhi(InnerLoop, V);
+        Loop *InnerMostLoop = InnerLoop;
+        while (!InnerMostLoop->getSubLoops().empty()) {
----------------
[Nit] `innermost` could be considered a single word


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:790-792
+        while (!InnerMostLoop->getSubLoops().empty()) {
+          InnerMostLoop = InnerMostLoop->getSubLoops().front();
+        }
----------------
[style] https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

I think the name `findInnerReductionPhi` implies that the search for the innermost loop happens in that function. Either that, or rename it to `findReductionPhi`.

`findInnerReductionPhi` is already a static helper function, you could wrap the new functionality into such a helper function as well.

[serious] This uses `InnerMostLoop->getSubLoops().front()` even if there are multiple nested loops inside (that hence do not even particpate in the loop interchange). Both loops might participate in the reduction.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:797-798
+        Loop *CurLoop = InnerMostLoop;
+        if (InnerRedPhi && InnerRedPhi->getParent() != InnerLoop->getHeader()) {
+          while (InnerRedPhi->getParent() != InnerLoop->getHeader()) {
+            InnerRedPhi = cast<PHINode>(InnerRedPhi->getIncomingValueForBlock(
----------------
The while condition and its if-guard is essentially the same condition and can be integrated.

I was also thinking about how `InnerRedPhi->getParent()` represents the `CurLoop` and can be directly compared `InnerLoop` instead of its header blocks, but it's the header of the parent loop. 
Its dependence on just loops may cause it to break when a condition is involved:

```
unsigned f(int e[100][100][100], int f[100][100][100]) {
   unsigned x = 0;
   for (int a = 0; a < 100; a++) {
     for (int b = 0; b < 100; b++) {
       if (c)
         for (int c = 0; c < 100; c++) // Not participating in the loop interchange
            x += e[c][b][a];
         } else {
           for (int c = 0; c < 100; c++) 
              x += 1;
        }
     }
   }
   return x;
 }
```
[serious] Here may want to interchange `a` and `b`. `b` has two subloops where your first `while` will just randomly pick one of them.  Then, there is another PHINode joining the result of the two `x` where `getIncomingValueForBlock` will be called with the wrong incoming BB and fail.

[serious] Another problem is that it does not check for what the value for the other incoming block is, it could be something unrelated to a reduction
```
for (int a = 0; a < 100; a++) 
  for (int b = 0; b < 100; b++) {
    for (int c = 0; c < n; c++)
      new_x = x = e[c][b][a];
    x = PHINode(42, new_x); // should be: PHINode(x, new_x); where `x` is the value when the c-loop has zero iterations and hence skipped by the loop guard.
  }
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125805/new/

https://reviews.llvm.org/D125805