Skip to content

[LoopInterchange] Move some processes to another function (NFC) #129514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

kasuga-fj
Copy link
Contributor

Some post-processing involved in exchanging a pair of loops has been done separately from processLoop, which is a main function that does the transformation. It's better to consolidate these processes into the same function. This patch is a preparation of #127474.

Some post-processing involved in exchanging a pair of loops has been
done separately from `processLoop`, which is a main function that does
the transformation. It's better to consolidate these processes into the
same function. This patch is a preparation of llvm#127474.
@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Ryotaro Kasuga (kasuga-fj)

Changes

Some post-processing involved in exchanging a pair of loops has been done separately from processLoop, which is a main function that does the transformation. It's better to consolidate these processes into the same function. This patch is a preparation of #127474.


Full diff: https://github.com/llvm/llvm-project/pull/129514.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/LoopInterchange.cpp (+16-15)
diff --git a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
index f45d90ff13e14..50a527c1a9247 100644
--- a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
@@ -508,21 +508,11 @@ struct LoopInterchange {
     // and do interchange based on a bubble-sort fasion. We start from
     // the innermost loop, move it outwards to the best possible position
     // and repeat this process.
-    for (unsigned j = SelecLoopId; j > 0; j--) {
+    for (unsigned J = SelecLoopId; J > 0; J--) {
       bool ChangedPerIter = false;
-      for (unsigned i = SelecLoopId; i > SelecLoopId - j; i--) {
-        bool Interchanged = processLoop(LoopList[i], LoopList[i - 1], i, i - 1,
-                                        DependencyMatrix, CostMap);
-        if (!Interchanged)
-          continue;
-        // Loops interchanged, update LoopList accordingly.
-        std::swap(LoopList[i - 1], LoopList[i]);
-        // Update the DependencyMatrix
-        interChangeDependencies(DependencyMatrix, i, i - 1);
-
-        LLVM_DEBUG(dbgs() << "Dependency matrix after interchange:\n";
-                   printDepMatrix(DependencyMatrix));
-
+      for (unsigned I = SelecLoopId; I > SelecLoopId - J; I--) {
+        bool Interchanged =
+            processLoop(LoopList, I, I - 1, DependencyMatrix, CostMap);
         ChangedPerIter |= Interchanged;
         Changed |= Interchanged;
       }
@@ -534,10 +524,12 @@ struct LoopInterchange {
     return Changed;
   }
 
-  bool processLoop(Loop *InnerLoop, Loop *OuterLoop, unsigned InnerLoopId,
+  bool processLoop(SmallVectorImpl<Loop *> &LoopList, unsigned InnerLoopId,
                    unsigned OuterLoopId,
                    std::vector<std::vector<char>> &DependencyMatrix,
                    const DenseMap<const Loop *, unsigned> &CostMap) {
+    Loop *OuterLoop = LoopList[OuterLoopId];
+    Loop *InnerLoop = LoopList[InnerLoopId];
     LLVM_DEBUG(dbgs() << "Processing InnerLoopId = " << InnerLoopId
                       << " and OuterLoopId = " << OuterLoopId << "\n");
     LoopInterchangeLegality LIL(OuterLoop, InnerLoop, SE, ORE);
@@ -566,6 +558,15 @@ struct LoopInterchange {
     LoopsInterchanged++;
 
     llvm::formLCSSARecursively(*OuterLoop, *DT, LI, SE);
+
+    // Loops interchanged, update LoopList accordingly.
+    std::swap(LoopList[OuterLoopId], LoopList[InnerLoopId]);
+    // Update the DependencyMatrix
+    interChangeDependencies(DependencyMatrix, InnerLoopId, OuterLoopId);
+
+    LLVM_DEBUG(dbgs() << "Dependency matrix after interchange:\n";
+               printDepMatrix(DependencyMatrix));
+
     return true;
   }
 };

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would prefer if you would leave existing variables names untouched, even if they do not strictly conform to LLVM coding standard (J, I)

@kasuga-fj
Copy link
Contributor Author

I would prefer if you would leave existing variables names untouched, even if they do not strictly conform to LLVM coding standard (J, I)

I reverted this change, thanks.

@kasuga-fj kasuga-fj merged commit aa37a69 into llvm:main Mar 4, 2025
11 checks passed
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
…#129514)

Some post-processing involved in exchanging a pair of loops has been
done separately from `processLoop`, which is a main function that does
the transformation. It's better to consolidate these processes into the
same function. This patch is a preparation of llvm#127474.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants