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

[Whitney Tsang via Phabricator via llvm-commits]

Whitney 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;
----------------
Meinersbur wrote:
> 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.
Then I think I misunderstood, currently CurLoop is only set when both instructions belongs to the same loop.


================
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>(
----------------
Meinersbur wrote:
> 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.
> 
Added code to allow accesses of different base.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:713-714
+                     LoopDepth + 1, CurLoop->getLoopDepth() + 1)) {
+              if (D->isScalar(CurLoopDepth))
+                continue;
+
----------------
Meinersbur wrote:
> 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 (=>,>)
> 
Why the above example cannot be jammed?
Before jam:
```
for i = ...
  for j = ...
    sum = f(sum, ...);
  for j = ...
    sum = f(sum, ...);
```
After jam:
```
for i = ...
  for j = ...
    sum = f(sum, ...);
    sum = f(sum, ...);
```
The access pattern seems to be the same to me.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132