[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:760-761
+      SmallVector<Value *, 4> DstLoadsAndStores;
+      if (!getLoadsAndStores(*I, SrcLoadsAndStores) ||
+          !getLoadsAndStores(*J, DstLoadsAndStores))
+        return false;
----------------
dmgreen wrote:
> 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.
Renamed, and removed the calculation of one of the LoadsAndStores in the outer loop. I considered creating a map, but the code may get complicated. Is the current change okay with you, or you prefer a map?


================
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.
----------------
dmgreen wrote:
> 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.
fixed the typo, and added braces, is not any clearer?


================
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();
----------------
dmgreen wrote:
> 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.
Not sure I understand, this check is already like before. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132