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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Nice work!

Do you have a test case?

I need to think about how dependencies violation are detected. Could you write something about how you intent to do it?



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:441
 
-  // The loop unroll and jam pass requires loops to be in simplified form, and also needs LCSSA.
-  // Since simplification may add new inner loops, it has to run before the
----------------
Whitney wrote:
> Just clang-format
Could you commit this separately?

I.e. just push a NFC commit explaining you are working on this file.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:101
 
+static bool partitionOuterLoopBlocks(
+    Loop &Root, Loop &JamLoop, BasicBlockSet &JamLoopBlocks,
----------------
Could you add a doxygen description for this function?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:118
+
+/// TODO Remove when UnrollAndJamLoop changed to support unroll and jamming more
+/// than 2 levels loop.
----------------
[nit] TODO's should not be doxygen comments


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:709
+            for (unsigned CurLoopDepth = LoopDepth + 1;
+                 CurLoopDepth <= CurLoop->getLoopDepth(); ++CurLoopDepth) {
+              if (D->getDirection(CurLoopDepth) & Dependence::DVEntry::LT) {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | Don't evaluate `Cur->getLoopDepth()` every time through the loop ]]. Consider using `llvm::seq`


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:716-717
+              }
+              if (D->getDirection(CurLoopDepth) & Dependence::DVEntry::GT)
+                break;
+            }
----------------
Could you write a comment about what kind of dependencies this is looking for/are not allowed?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:745
+  for (auto *I = AllBlocks.begin(); I != LastIterator; ++I) {
+    for (auto *J = I; J != AllBlocks.end(); ++J) {
+      if (I == J && (*I == ForeBlocksMap.lookup(&Root) ||
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | Don’t evaluate `end()` every time through a loop ]]


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:757
+      if (!checkDependencies(SrcLoadsAndStores, DstLoadsAndStores, LoopDepth,
+                             (I == J) ? LI.getLoopFor(*(*I).begin()) : nullptr,
+                             DI))
----------------
[style] `(*I).begin()` -> `I->begin()`?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:766
+
+static bool isEligibleLoopForm(const Loop &Root) {
+  // Root must have a child.
----------------
Can the following be handled correctly?
 * Multiple exits



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:777
+
+    if (!L->isRotatedForm())
+      return false;
----------------
Why is rotated form necessary?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:810
+
   /* We currently handle outer loops like this:
         |
----------------
Does the comment need updating?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132