[PATCH] D120386: [LoopInterchange] Try to achieve the most optimal access pattern after interchange

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

Thanks a lot for the comments @Meinersbur! I've updated the patch accordingly.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:284
+    if (Vec->size() != 1) {
+      LoopList = {};
+      return;
----------------
Meinersbur wrote:
> If you don't assume the vector is empty when starting (`assert(LoopList.empty())`?), consider clearing the list at the start of the function.
> 
> Change seems NFC anyway.
Thanks for the comment, I added the assert correspondingly.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:420
 
+  SmallVector<Loop *, 8> LoopList = {};
+
----------------
Meinersbur wrote:
> Why make it an object member?
You are correct that it may not be necessary to make it an object member, I moved `LoopList` to inside the `run()` function.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:508
     // Move the selected loop outwards to the best possible position.
-    Loop *LoopToBeInterchanged = LoopList[SelecLoopId];
-    for (unsigned i = SelecLoopId; i > 0; i--) {
-      bool Interchanged = processLoop(LoopToBeInterchanged, LoopList[i - 1], i,
-                                      i - 1, DependencyMatrix);
-      if (!Interchanged)
-        return Changed;
-      // Update the DependencyMatrix
-      interChangeDependencies(DependencyMatrix, i, i - 1);
+    for (unsigned j = SelecLoopId; j > 0; j--) {
+      for (unsigned i = SelecLoopId; i > SelecLoopId - j; i--) {
----------------
Meinersbur wrote:
> Could you add the motivation to use bubble sort to the comment here?
> 
> Any particular reason to start with moving the innermost loops inwards (decreasing `j`) instead the other way around?
Thanks, comments updated.

There is no particular reason that we start with moving the innermost loops outwards. Previously loop interchange picks the innermost loop and try our best to move it outwards, so in this patch I started with the innermost loop as well, to make it more "consistent" with the previous repo.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:513
+        if (!Interchanged)
+          continue;
+        // Loops interchanged, update LoopList accordingly.
----------------
Meinersbur wrote:
> There should also be an early abort if there was no interchange during an entire round of `i` (like https://en.wikipedia.org/wiki/Bubble_sort#Pseudocode_implementation : `until not swapped`)
Thanks, I added an early abort accordingly.


================
Comment at: llvm/test/Transforms/LoopInterchange/phi-ordering.ll:12
 ; Function Attrs: norecurse nounwind
-define void @test(i32 %T, [90 x i32]* noalias nocapture %C, i16* noalias nocapture readonly %A, i16* noalias nocapture readonly %B) local_unnamed_addr #0 {
+define void @test(i32 %T, [90 x i32]* noalias nocapture %C, [90 x [90 x i16]]* noalias nocapture readonly %A, i16* noalias nocapture readonly %B) local_unnamed_addr #0 {
 ; CHECK-LABEL: @test(
----------------
Meinersbur wrote:
> Why changing this test?
The original test ignores loop interchange cost model (with -loop-interchange-threshold=-1000) and does interchange two times (which does not generate good memory access pattern).

If I did not change the test, with the bubble sort fashion it would interchange every neighboring loops due to ignored cost model, and will eventually interchange three times generating a completely different output as compared to the original test case.

If I only set `-loop-interchange-threshold=0`, it would interchange only once which does give good memory access pattern, but the output would still be quite different from the current test.

Therefore I also changed array @A from 1-dimension to 2-dimension, thus it does interchange twice, and generated an output that is as close to the original one as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120386