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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a reviewer: fhahn.
Meinersbur added inline comments.
Herald added a project: All.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:284
+    if (Vec->size() != 1) {
+      LoopList = {};
+      return;
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:420
 
+  SmallVector<Loop *, 8> LoopList = {};
+
----------------
Why make it an object member?


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


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:513
+        if (!Interchanged)
+          continue;
+        // Loops interchanged, update LoopList accordingly.
----------------
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`)


================
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(
----------------
Why changing this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120386