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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:760-761
+      SmallVector<Value *, 4> DstLoadsAndStores;
+      if (!getLoadsAndStores(*I, SrcLoadsAndStores) ||
+          !getLoadsAndStores(*J, DstLoadsAndStores))
+        return false;
----------------
This looks like it would recalculate LoadsAndStores a lot? Src and Dst don't seem like the right nomenclature too, but that's just a small nit.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:808
+static Loop *getInnerMostLoop(Loop *L) {
+  while (L->getSubLoops().size() != 0)
+    L = L->getSubLoops()[0];
----------------
`size() != 0` -> `!empty()` ?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:860
+           F11 F21 S1_1 S1_2 A21 A11 F12 F22 S2_1 S2_2 A22 A12.
+         It needs to be safe to tranform this to
+           F11 F12 F21 F22 S1_1 S2_1 S1_2 S2_2 A21 A22 A11 A12.
----------------
I realize you didn't write the original, but tranform -> transform

I believe the old message was intending to show array indices, so Fi was F(i) and Si_j was S(i,j), showing that the second unrolled iteration need to be able to move past the first, in terms of runtime execution.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:914
   // Make sure we can move all instructions we need to before the subloop
+  BasicBlock *Header = L->getHeader();
+  BasicBlock *Latch = L->getLoopLatch();
----------------
Should this have a check for a 2 deep loop nest at the moment (like before), if the remainder of the analysis/transform code hasn't been updated yet? It looks like the count calculation might just exclude anything with multiple subloop blocks at the moment anyway, so is possibly not a problem in practice, without pragma's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132