[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: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).
----------------
i don't see this being enforced.


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

  ...
}
```



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


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


================
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"
----------------
Again, the `&` comparison feels wrong. Did you consider a switch over the values of `Dependence::DVEntry`?


================
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
+
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132