[PATCH] D76132: [LoopUnrollAndJam] Changed safety checks to consider more than 2-levels loop nest.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:703-706
             LLVM_DEBUG(dbgs() << "  > dependency between:\n"
                               << "  " << *Src << "\n"
                               << "  " << *Dst << "\n");
             return false;
----------------
Whitney wrote:
> Meinersbur wrote:
> > This looks like a bail-out; might improve this by setting `CurLoop` to innermost loop both instructions are in.
> > 
> > I'd prefer the following structure:
> > ```
> > if (D->getDirection(LoopDepth) & Dependence::DVEntry::GT) { // That is, as by the other comment, check whter the root loop carries this dependency
> >   if (!CurLoop)
> >     return false; // Bail-out
> > 
> >   ...
> > }
> > ```
> > 
> > might improve this by setting CurLoop to innermost loop both instructions are in.
> 
> Do you mean passing `LI` into this function, and calculating `CurLoop` here?
Where the innermost common loop is computed is unimportant.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:710
+          assert(CurLoop->getLoopDepth() <= D->getLevels());
+          if (D->getDirection(LoopDepth) & Dependence::DVEntry::GT) {
+            for (unsigned CurLoopDepth : llvm::seq<unsigned>(
----------------
Whitney wrote:
> Meinersbur wrote:
> > I think this is should be looking for whether the first `GT` direction is due to the root loop (i.e. whether the root loop is the cause for the dependence to be fulfilled). Not an correctness issue.
> > 
> > The bitwise `&` comparison also matches `NE` and `GE`. Is this intended?
> > I think this is should be looking for whether the first GT direction is due to the root loop (i.e. whether the root loop is the cause for the dependence to be fulfilled). Not an correctness issue.
> 
> Here `LoopDepth` is assumed to be the loop depth of the unroll loop.
> 
> > The bitwise & comparison also matches NE and GE. Is this intended?
> 
> The code before my change is using `&`, so I don't want to change the behaviour. 
> Here LoopDepth is assumed to be the loop depth of the unroll loop.

I know. However, the dependency might also be fullfiled by surrounding loops. eg:
```
for i = 0 ... n-1
  #pragma unrollandjam
  for j = 0 ... n-1
    for j = 0 ... n-1:
      A[i][j][k] = f(A[i-1][j-1][k]);
```
The dependence vector is (>,>,=).
Verifying the direction of the j-dependence is not necessary because for the same `i`, the inner loops are parallel. In other words: the `i`-loop already ensures that the dependency is fulfilled.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:713-714
+                     LoopDepth + 1, CurLoop->getLoopDepth() + 1)) {
+              if (D->isScalar(CurLoopDepth))
+                continue;
+
----------------
Whitney wrote:
> Meinersbur wrote:
> > Isn't this equivalent to `D->getDirection(CurLoopDepth ) == EQ`?
> Could also be `ALL`. I think `isScalar` is clearer. 
`ALL` would be `*` in the dependence vector, i.e. the analysis could not prove any direction.
However, it's also not `EQ`. Sorry for the confusion.

I am not convinced it can be ignored.

```
#pragma unrollandjam
for i = ...
  for j = ...
    sum = f(sum, ...);
```
The dependence induced by sum is obviously scalar, but it cannot be jammed (it's sequential). The dependence vector is (=>,>)



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:717
+              // If the dependence direction is LT, then it is not safe to fuse.
+              if (D->getDirection(CurLoopDepth) & Dependence::DVEntry::LT) {
+                LLVM_DEBUG(dbgs() << "  < > dependency between:\n"
----------------
Whitney wrote:
> Meinersbur wrote:
> > Again, the `&` comparison feels wrong. Did you consider a switch over the values of `Dependence::DVEntry`?
> The code before my change is using `&`.  I considered changing to use dependence distance instead of direction to allow more dependence, but I want to keep this patch as changes needed for safety checks for more than 2-levels loop nest. 
> There is a FIXME: `// FIXME: Allow > so long as distance is less than unroll width`
> What do you think?
I think taking the dependence distance into account is something for a different patch.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:726
+              // InnerLoopDepth would be GT.
+              if (D->getDirection(CurLoopDepth) & Dependence::DVEntry::GT)
+                break;
----------------
It could also be `ALL` (or `GE`), meaning we don't know what direction it is. This check gives it a pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132