[LoopInterchange] Ignore the cost-model, force interchange if legal #148858

sjoerdmeijer · 2025-07-15T14:26:37Z

This is and has been proven useful for testing purposes, to get more test coverage.

This is useful for testing purposes, to get more test coverage.

llvmbot · 2025-07-15T14:27:09Z

@llvm/pr-subscribers-llvm-transforms

Author: Sjoerd Meijer (sjoerdmeijer)

Changes

This is and has been proven useful for testing purposes, to get more test coverage.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/LoopInterchange.cpp (+6-1)
(modified) llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll (+4)

diff --git a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
index a5008907b9014..b62c25d52861b 100644
--- a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
@@ -66,6 +66,10 @@ static cl::opt<unsigned int> MaxMemInstrCount(
         "in the dependency matrix. Higher value may lead to more interchanges "
         "at the cost of compile-time"));
 
+static cl::opt<bool> ForceLoopInterchange(
+    "loop-interchange-force", cl::init(false), cl::Hidden,
+    cl::desc("Ignore the cost model, and force interchange if it is legal"));
+
 namespace {
 
 using LoopVector = SmallVector<Loop *, 8>;
@@ -599,7 +603,8 @@ struct LoopInterchange {
     }
     LLVM_DEBUG(dbgs() << "Loops are legal to interchange\n");
     LoopInterchangeProfitability LIP(OuterLoop, InnerLoop, SE, ORE);
-    if (!LIP.isProfitable(InnerLoop, OuterLoop, InnerLoopId, OuterLoopId,
+    if (!ForceLoopInterchange &&
+        !LIP.isProfitable(InnerLoop, OuterLoop, InnerLoopId, OuterLoopId,
                           DependencyMatrix, CCM)) {
       LLVM_DEBUG(dbgs() << "Interchanging loops not profitable.\n");
       return false;
diff --git a/llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll b/llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll
index 16952a66aa78e..59196bbbb9c00 100644
--- a/llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll
+++ b/llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll
@@ -2,6 +2,10 @@
 ; RUN:     -pass-remarks-output=%t -disable-output
 ; RUN: FileCheck -input-file %t --check-prefix=PROFIT-CACHE %s
 
+; RUN: opt < %s -passes=loop-interchange -cache-line-size=64 \
+; RUN:     -pass-remarks-output=%t -disable-output -loop-interchange-force=true
+; RUN: FileCheck -input-file %t --check-prefix=PROFIT-VEC %s
+
 ; RUN: opt < %s -passes=loop-interchange -cache-line-size=64 \
 ; RUN:     -pass-remarks-output=%t -disable-output -loop-interchange-profitabilities=vectorize,cache,instorder
 ; RUN: FileCheck -input-file %t --check-prefix=PROFIT-VEC %s

madhur13490 · 2025-07-15T14:51:48Z

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

+static cl::opt<bool> ForceLoopInterchange(
+    "loop-interchange-force", cl::init(false), cl::Hidden,
+    cl::desc("Ignore the cost model, and force interchange if it is legal"));
+


Would it be better if we call it -loop-interchange-ignore-cost-model? -force seems it covers wider scenarios.

kasuga-fj

What do you think about adding something like force here instead? I personally think it could be useful in some cases, e.g., we can specify -loop-interchange-profitabilities=cache,force. The behavior is slightly different from -loop-interchange-profitabilities=cache. The former will interchange the loops if it is not unprofitable for the cache, while the latter will perform the interchange if it is profitable for the cache. The result will differ in cases where it is unclear whether interchanging is profitable or unprofitable for the cache.

kasuga-fj · 2025-07-15T16:04:29Z

llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll

+; RUN: opt < %s -passes=loop-interchange -cache-line-size=64 \
+; RUN:     -pass-remarks-output=%t -disable-output -loop-interchange-force=true
+; RUN: FileCheck -input-file %t --check-prefix=PROFIT-VEC %s


Could you please add another test case? Interchanging the loops in this test would be beneficial in perspective of vectorizatoin (i.e., isProfitableForVectorization returns true in this case) because it makes the innermost loop vectorizable. It would be ideal to add a case where interchanging is definitely unprofitable.

sjoerdmeijer · 2025-07-15T18:49:21Z

What do you think about adding something like force here instead? I personally think it could be useful in some cases, e.g., we can specify -loop-interchange-profitabilities=cache,force. The behavior is slightly different from -loop-interchange-profitabilities=cache. The former will interchange the loops if it is not unprofitable for the cache, while the latter will perform the interchange if it is profitable for the cache. The result will differ in cases where it is unclear whether interchanging is profitable or unprofitable for the cache.

Yeah, I don't mind adding it there, that makes sense. I am a little bit undecided, but I am thinking that the combination cache,force is not a very intuitive option to read, it is not very clear what that means. My question is if we can come up with something that is clearer.

But I see what you would like to achieve, so let me summarise the kind of 3 states that we could have:

I would like an option to ignore the cost-model entirely, i.e. to force interchange.
Your suggestion is to prefer interchange if the cost-model is undecided. That makes sense.
and what we already have, to have it entirely driven by the cost-model (what we currently have, nothing changes for this case).

The first one could be achieved by -loop-interchange-profitabilities=force, so I will add that one first, to help my test coverage.

I might do this in a follow up with another patch to address the issue when the cost-model is undecided. I am not totally in love with this, but I think I will add options:

-loop-interchange-profitabilities=cache-force
-loop-interchange-profitabilities=vectorize-force
-loop-interchange-profitabilities=instorder-force

to prefer interchange when the profitability is not clear. I think a separate option cache-force is a bit clearer than a comma separate list and giving a special meaning to the previous option.

kasuga-fj · 2025-07-15T19:35:30Z

Sorry, I spaced out and forgot to mention what I’m actually concerned about: the behavior when both --loop-interchange-profitabilities=... and --loop-interchange-force=1 are specified. Which one should take precedence in this case? It depends on the implementation. Of course, I think it’s fine to explicitly describe the priority between these options somewhere, but instead of providing two separate options, I’d prefer to have a single option to avoid such complications.

Being able to specify something like --loop-interchange-profitabilities=cache,force is just a small bonus, and doesn’t seem very important to me.

sjoerdmeijer · 2025-07-16T13:11:37Z

the behavior when both --loop-interchange-profitabilities=... and --loop-interchange-force=1 are specified. Which one should take precedence in this case?

I think that should be forcing interchange, which should be the current behaviour. But I am adding force as an option to --loop-interchange-profitabilities= because I like the idea of having one interface for controlling the cost model. In addition, it solves this problem/unclarity.

github-actions · 2025-07-17T13:00:51Z

✅ With the latest revision this PR passed the C/C++ code formatter.

there.

kasuga-fj · 2025-07-17T13:14:27Z

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

+      // TODO? We ignore the force option when it appears in a list, i.e. it
+      // should occur as the only option to be effective, as mentioned in the
+      // help.


Suggested change

// TODO? We ignore the force option when it appears in a list, i.e. it

// should occur as the only option to be effective, as mentioned in the

// help.

shouldInterchange = true;

kasuga-fj · 2025-07-17T13:15:53Z

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

+
+  // Return true if interchange is forced.
+  if (Profitabilities.size() == 1 && Profitabilities[0] == RuleTy::Ignore)
+    return true;
+


Unnecessary if we handle RuleTy::Ignore properly in the later switch statement.

llvm/test/Transforms/LoopInterchange/profitability-vectorization.ll

llvm/test/Transforms/LoopInterchange/force-interchange.ll

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

Co-authored-by: Ryotaro Kasuga <[email protected]>

kasuga-fj

orce interchange can occur odd behavior in some cases. Can you add a test to show it? Here is an example:

int A[4][4][4];
for (int i = 0; i < 4; i++)
  for (int j = 1; j < 4; j++)
    for (int k = 0; k < 4; k++)
      A[3-i][j-1][k] = A[i][j][k];

IIUIC, the following scenario happens due to the bubble-sort based heuristic:

Try to interchange the 2nd loop (j-loop) and 3rd loop (k-loop). It is legal, so they are interchanged.
Try to interchange the 1st loop (i-loop) and 2nd loop (k-loop, because the j-loop and k-loop were interchanged). It is illegal, so they are NOT interchanged.
Try to interchange the 2nd loop (k-loop) and 3rd loop (j-loop). It is legal indeed, so they are interchanged again.

Consequently, the first interchange is reverted, and the final loop order remains the same as the original. I believe this is acceptable, considering that the force interchange option is intended for testing purposes. However, it is worth clarifying this behavior with an example.

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

llvm/test/Transforms/LoopInterchange/force-interchange.ll

kasuga-fj

LGTM. Please consider to simplify the test, as @fhahn mentioned.

kasuga-fj · 2025-07-18T09:14:42Z

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

@@ -1311,6 +1324,9 @@ bool LoopInterchangeProfitability::isProfitable(
      shouldInterchange =
          isProfitableForVectorization(InnerLoopId, OuterLoopId, DepMatrix);
      break;
+    case RuleTy::Ignore:
+      // Nothing to do, this has no effect.


Maybe using llvm_unreachable is better?

kasuga-fj · 2025-07-18T09:17:39Z

llvm/test/Transforms/LoopInterchange/force-interchange.ll

@@ -0,0 +1,43 @@
+; RUN: opt < %s -passes=loop-interchange -pass-remarks-output=%t -disable-output -loop-interchange-profitabilities=ignore -S


orce interchange can occur odd behavior in some cases. Can you add a test to show it? Here is an example:

int A[4][4][4]; for (int i = 0; i < 4; i++) for (int j = 1; j < 4; j++) for (int k = 0; k < 4; k++) A[3-i][j-1][k] = A[i][j][k];

IIUIC, the following scenario happens due to the bubble-sort based heuristic:

1. Try to interchange the 2nd loop (j-loop) and 3rd loop (k-loop). It is legal, so they are interchanged. 2. Try to interchange the 1st loop (i-loop) and 2nd loop (k-loop, because the j-loop and k-loop were interchanged). It is illegal, so they are NOT interchanged. 3. Try to interchange the 2nd loop (k-loop) and 3rd loop (j-loop). It is legal indeed, so they are interchanged again.

Consequently, the first interchange is reverted, and the final loop order remains the same as the original. I believe this is acceptable, considering that the force interchange option is intended for testing purposes. However, it is worth clarifying this behavior with an example.

Could you please add this? It's fine to do it in another PR.

Ok, thanks, I will commit this and look at that suggestion early next week.
This allows me to run more testing over the weekend.
I have already found a couple of interchange problems, but can raise issues when this option is available.

[LoopInterchange] Ignore the cost-model, force interchange if legal

db9c37e

This is useful for testing purposes, to get more test coverage.

sjoerdmeijer requested review from fhahn, madhur13490 and kasuga-fj July 15, 2025 14:26

llvmbot added the llvm:transforms label Jul 15, 2025

madhur13490 reviewed Jul 15, 2025

View reviewed changes

kasuga-fj reviewed Jul 15, 2025

View reviewed changes

Add an ignore value to the existing profitability option.

fe979ea

sjoerdmeijer added 2 commits July 17, 2025 06:06

Remove the old option, we don't need that anymore, left it accidentally

5bb2158

there.

Fix formatting.

1e8e667

kasuga-fj reviewed Jul 17, 2025

View reviewed changes

llvm/lib/Transforms/Scalar/LoopInterchange.cpp Show resolved Hide resolved

sjoerdmeijer and others added 2 commits July 17, 2025 14:33

Update llvm/test/Transforms/LoopInterchange/force-interchange.ll

6a3b3f0

Co-authored-by: Ryotaro Kasuga <[email protected]>

Addressed review comments

82a3043

kasuga-fj reviewed Jul 17, 2025

View reviewed changes

llvm/lib/Transforms/Scalar/LoopInterchange.cpp Show resolved Hide resolved

Debug message when ignore has no effect.

58a6efb

kasuga-fj reviewed Jul 18, 2025

View reviewed changes

llvm/lib/Transforms/Scalar/LoopInterchange.cpp Outdated Show resolved Hide resolved

noDuplicateRulesAndIgnore: assert that ignore doesn't appear in a list.

057d32b

fhahn reviewed Jul 18, 2025

View reviewed changes

llvm/test/Transforms/LoopInterchange/force-interchange.ll Outdated Show resolved Hide resolved

Renamed test case labels and variables.

b5bc6d1

kasuga-fj approved these changes Jul 18, 2025

View reviewed changes

Added llvm_unreachable

1736f1f

sjoerdmeijer merged commit a9f8143 into llvm:main Jul 18, 2025
9 checks passed

		@@ -0,0 +1,43 @@
		; RUN: opt < %s -passes=loop-interchange -pass-remarks-output=%t -disable-output -loop-interchange-profitabilities=ignore -S

[LoopInterchange] Ignore the cost-model, force interchange if legal #148858

[LoopInterchange] Ignore the cost-model, force interchange if legal #148858

Uh oh!

Conversation

sjoerdmeijer commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

madhur13490 Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

sjoerdmeijer commented Jul 15, 2025

Uh oh!

kasuga-fj commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjoerdmeijer commented Jul 16, 2025

Uh oh!

github-actions bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasuga-fj Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sjoerdmeijer Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kasuga-fj commented Jul 15, 2025 •

edited

Loading

github-actions bot commented Jul 17, 2025 •

edited

Loading