[PATCH] D125805: [LoopInterchange] Support multi-level reduction loops
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 29 13:46:20 PDT 2022
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
More information about the llvm-commits
mailing list