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

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added a comment.

> Harbormaster failed remote builds in B49526 <https://reviews.llvm.org/B49526>: Diff 250956!

Looks like some llc test cases failed. Doesn't seem related to this patch to me. Anyone know if it is safe to ignore?



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:659-661
       // Track dependencies, and if we find them take a conservative approach
       // by allowing only = or < (not >), altough some > would be safe
       // (depending upon unroll width).
----------------
Meinersbur wrote:
> i don't see this being enforced.
`if (D->getDirection(LoopDepth) & Dependence::DVEntry::GT) return false;` => `allowing only = or < (not >)`


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:703-706
             LLVM_DEBUG(dbgs() << "  > dependency between:\n"
                               << "  " << *Src << "\n"
                               << "  " << *Dst << "\n");
             return false;
----------------
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?


================
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:
> 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. 


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:713-714
+                     LoopDepth + 1, CurLoop->getLoopDepth() + 1)) {
+              if (D->isScalar(CurLoopDepth))
+                continue;
+
----------------
Meinersbur wrote:
> Isn't this equivalent to `D->getDirection(CurLoopDepth ) == EQ`?
Could also be `ALL`. I think `isScalar` is clearer. 


================
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"
----------------
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?


================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/dependencies.ll:518
+  %cmp.j = icmp slt i32 %inc.j, 100
+  br i1 %cmp.j, label %for.j, label %for.i.latch, !llvm.loop !1
+
----------------
Meinersbur wrote:
> Does it otherwise unroll-and-jam the middle loop? Should we add a mechanism that stops unrolling of nests of already unrolled loops (e.g. add `llvm.loop.unroll_and_jam.disable` to all nested loops)?
Currently unroll and jam add `llvm.loop.unroll_and_jam.disable` to the loop it unroll and jammed (not all nested loops).
As we are traversing from inner to outer, 
`for.k` is not considered as it doesn't have a inner loop
`for.j` is safe to unroll and jam, but is not my intension for this test case, so I added the disable pragma
`for.i` is unsafe only after my change. 

Even if we change to traverse from outer to inner, we should allow the middle loop to unroll and jam when proven profitable. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132