[LV] Return Invalid from getLegacyCost when instruction cost forced. #154543

kmclaughlin-arm · 2025-08-20T13:53:52Z

LoopVectorizationCostModel::expectedCost will only override the cost
returned by getInstructionCost when valid. This patch ensures we do
the same in VPCostContext::getLegacyCost, avoiding the "VPlan cost
model and legacy cost model disagreed" assert in the included test.

LoopVectorizationCostModel::expectedCost will only override the cost returned by getInstructionCost when valid. This patch ensures we do the same in VPCostContext::getLegacyCost, avoiding the "VPlan cost model and legacy cost model disagreed" assert in the included test.

llvmbot · 2025-08-20T13:54:29Z

@llvm/pr-subscribers-vectorizers

Author: Kerry McLaughlin (kmclaughlin-arm)

Changes

LoopVectorizationCostModel::expectedCost will only override the cost
returned by getInstructionCost when valid. This patch ensures we do
the same in VPCostContext::getLegacyCost, avoiding the "VPlan cost
model and legacy cost model disagreed" assert in the included test.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+3-2)
(added) llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll (+24)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 70f884016d08c..2d86b2b27f5f6 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -6760,9 +6760,10 @@ void LoopVectorizationPlanner::plan(ElementCount UserVF, unsigned UserIC) {
 
 InstructionCost VPCostContext::getLegacyCost(Instruction *UI,
                                              ElementCount VF) const {
-  if (ForceTargetInstructionCost.getNumOccurrences())
+  InstructionCost Cost = CM.getInstructionCost(UI, VF);
+  if (Cost.isValid() && ForceTargetInstructionCost.getNumOccurrences())
     return InstructionCost(ForceTargetInstructionCost.getNumOccurrences());
-  return CM.getInstructionCost(UI, VF);
+  return Cost;
 }
 
 bool VPCostContext::isLegacyUniformAfterVectorization(Instruction *I,
diff --git a/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll b/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll
new file mode 100644
index 0000000000000..52a72014392d7
--- /dev/null
+++ b/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll
@@ -0,0 +1,24 @@
+; REQUIRES: asserts
+; RUN: opt < %s -passes=loop-vectorize -force-target-instruction-cost=1 -debug-only=loop-vectorize -S -disable-output 2>&1 | FileCheck %s
+target triple = "aarch64-linux-gnu"
+
+define i32 @invalid_legacy_cost(i64 %N) #0 {
+; CHECK: LV: Checking a loop in 'invalid_legacy_cost
+; CHECK: LV: Found an estimated cost of Invalid for VF vscale x 2 For instruction: %0 = alloca i8, i64 0, align 16
+entry:
+  br label %for.body
+
+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %0 = alloca i8, i64 0, align 16
+  %arrayidx = getelementptr ptr, ptr null, i64 %iv
+  store ptr %0, ptr %arrayidx, align 8
+  %iv.next = add i64 %iv, 1
+  %exitcond.not = icmp eq i64 %iv, %N
+  br i1 %exitcond.not, label %for.end, label %for.body
+
+for.end:
+  ret i32 0
+}
+
+attributes #0 = { "target-features"="+neon,+sve" vscale_range(1,16) }

llvmbot · 2025-08-20T13:54:30Z

@llvm/pr-subscribers-llvm-transforms

Author: Kerry McLaughlin (kmclaughlin-arm)

Changes

LoopVectorizationCostModel::expectedCost will only override the cost
returned by getInstructionCost when valid. This patch ensures we do
the same in VPCostContext::getLegacyCost, avoiding the "VPlan cost
model and legacy cost model disagreed" assert in the included test.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+3-2)
(added) llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll (+24)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 70f884016d08c..2d86b2b27f5f6 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -6760,9 +6760,10 @@ void LoopVectorizationPlanner::plan(ElementCount UserVF, unsigned UserIC) {
 
 InstructionCost VPCostContext::getLegacyCost(Instruction *UI,
                                              ElementCount VF) const {
-  if (ForceTargetInstructionCost.getNumOccurrences())
+  InstructionCost Cost = CM.getInstructionCost(UI, VF);
+  if (Cost.isValid() && ForceTargetInstructionCost.getNumOccurrences())
     return InstructionCost(ForceTargetInstructionCost.getNumOccurrences());
-  return CM.getInstructionCost(UI, VF);
+  return Cost;
 }
 
 bool VPCostContext::isLegacyUniformAfterVectorization(Instruction *I,
diff --git a/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll b/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll
new file mode 100644
index 0000000000000..52a72014392d7
--- /dev/null
+++ b/llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll
@@ -0,0 +1,24 @@
+; REQUIRES: asserts
+; RUN: opt < %s -passes=loop-vectorize -force-target-instruction-cost=1 -debug-only=loop-vectorize -S -disable-output 2>&1 | FileCheck %s
+target triple = "aarch64-linux-gnu"
+
+define i32 @invalid_legacy_cost(i64 %N) #0 {
+; CHECK: LV: Checking a loop in 'invalid_legacy_cost
+; CHECK: LV: Found an estimated cost of Invalid for VF vscale x 2 For instruction: %0 = alloca i8, i64 0, align 16
+entry:
+  br label %for.body
+
+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %0 = alloca i8, i64 0, align 16
+  %arrayidx = getelementptr ptr, ptr null, i64 %iv
+  store ptr %0, ptr %arrayidx, align 8
+  %iv.next = add i64 %iv, 1
+  %exitcond.not = icmp eq i64 %iv, %N
+  br i1 %exitcond.not, label %for.end, label %for.body
+
+for.end:
+  ret i32 0
+}
+
+attributes #0 = { "target-features"="+neon,+sve" vscale_range(1,16) }

kmclaughlin-arm · 2025-08-20T13:54:52Z

This PR fixes another issue I found whilst testing #147535 with LNT.
When passing -force-target-instruction-cost=1, oggenc fails to build as it hits the "VPlan cost model and legacy cost model disagreed" assert.

lukel97

LGTM

david-arm · 2025-08-20T14:02:19Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+  InstructionCost Cost = CM.getInstructionCost(UI, VF);
+  if (Cost.isValid() && ForceTargetInstructionCost.getNumOccurrences())
    return InstructionCost(ForceTargetInstructionCost.getNumOccurrences());
-  return CM.getInstructionCost(UI, VF);


This is not a fault with your patch, but it looks like it was already broken due to this:

return InstructionCost(ForceTargetInstructionCost.getNumOccurrences());

I'm pretty sure it should be:

return InstructionCost(ForceTargetInstructionCost);

similar to LoopVectorizationCostModel::expectedCost

I think you're right, it should be ForceTargetInstructionCost. I've updated this in the latest commit.

Yes, to test this we need a test that passes a large value, e.g. -force-target-instruction-cost=100 in llvm/test/Transforms/LoopVectorize/AArch64/force-target-instruction-cost.ll would do it

fhahn · 2025-08-20T14:28:24Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

+
+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %0 = alloca i8, i64 0, align 16


Is there a way to test this without an in-loop alloca?

The alloca came from the original loop in oggenc where I found this issue and it's this instruction which leads to the assert.
I've tried to rewrite the loop using another instruction which will also trigger it, but I haven't found another way.

Ah right, it needs to be in the legacy path. Nevermind, thanks for checking

fhahn · 2025-08-20T14:28:44Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %0 = alloca i8, i64 0, align 16
+  %arrayidx = getelementptr ptr, ptr null, i64 %iv


please avoid storing to nullptr, with is UB.

fhahn · 2025-08-20T14:28:56Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

+for.end:
+  ret i32 0


Suggested change

for.end:

ret i32 0

for.end:

ret void

fhahn · 2025-08-20T14:29:30Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

@@ -0,0 +1,24 @@
+; REQUIRES: asserts
+; RUN: opt < %s -passes=loop-vectorize -force-target-instruction-cost=1 -debug-only=loop-vectorize -S -disable-output 2>&1 | FileCheck %s


Can you move the test to llvm/test/Transforms/LoopVectorize/AArch64/force-target-instruction-cost.ll, which aloready has similar tests

I've moved the test into force-target-instruction-cost.ll and changed the flag in that test to -force-target-instruction-cost=100 as suggested.

fhahn · 2025-08-20T14:30:04Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

+
+define i32 @invalid_legacy_cost(i64 %N) #0 {
+; CHECK: LV: Checking a loop in 'invalid_legacy_cost
+; CHECK: LV: Found an estimated cost of Invalid for VF vscale x 2 For instruction: %0 = alloca i8, i64 0, align 16


This is checking the legacy cost model print I think, you want to check the VPlan printing or just check the generated IR

Thanks, I'm now just checking the generated IR after moving the test.

fhahn · 2025-08-20T14:31:46Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+  InstructionCost Cost = CM.getInstructionCost(UI, VF);
+  if (Cost.isValid() && ForceTargetInstructionCost.getNumOccurrences())
    return InstructionCost(ForceTargetInstructionCost.getNumOccurrences());
-  return CM.getInstructionCost(UI, VF);


Yes, to test this we need a test that passes a large value, e.g. -force-target-instruction-cost=100 in llvm/test/Transforms/LoopVectorize/AArch64/force-target-instruction-cost.ll would do it

- Set -force-target-instruction-cost=100

david-arm

LGTM!

fhahn

LGTM, thanks

fhahn · 2025-08-21T20:27:52Z

llvm/test/Transforms/LoopVectorize/AArch64/force-target-instruction-cost.ll

@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt -p loop-vectorize -force-target-instruction-cost=1 -S %s | FileCheck %s
+; RUN: opt -p loop-vectorize -force-target-instruction-cost=100 -S %s | FileCheck %s


I know I originally suggested 100, but if a smaller value also works that would probably be good

fhahn · 2025-08-21T20:29:28Z

llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll

+
+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %0 = alloca i8, i64 0, align 16


Ah right, it needs to be in the legacy path. Nevermind, thanks for checking

kmclaughlin-arm requested review from david-arm, fhahn and lukel97 August 20, 2025 13:53

llvmbot added vectorizers llvm:transforms labels Aug 20, 2025

lukel97 approved these changes Aug 20, 2025

View reviewed changes

david-arm reviewed Aug 20, 2025

View reviewed changes

- Replace getNumOccurrences() with ForceTargetInstructionCost

e9ccb54

fhahn reviewed Aug 20, 2025

View reviewed changes

- Moved new test into force-target-instruction-cost.ll

4b4d4a5

- Set -force-target-instruction-cost=100

david-arm approved these changes Aug 21, 2025

View reviewed changes

fhahn approved these changes Aug 21, 2025

View reviewed changes

- Use -force-target-instruction-cost=10

2f14b65

kmclaughlin-arm merged commit 884c03e into llvm:main Aug 26, 2025
9 checks passed

aemerson mentioned this pull request Aug 30, 2025

[NFC] [clangd] [Modules] remove dot in log #156207

Closed

kmclaughlin-arm deleted the legacy-cost-invalid branch October 7, 2025 08:39

		@@ -0,0 +1,24 @@
		; REQUIRES: asserts
		; RUN: opt < %s -passes=loop-vectorize -force-target-instruction-cost=1 -debug-only=loop-vectorize -S -disable-output 2>&1 \| FileCheck %s

[LV] Return Invalid from getLegacyCost when instruction cost forced. #154543

[LV] Return Invalid from getLegacyCost when instruction cost forced. #154543

Uh oh!

Conversation

kmclaughlin-arm commented Aug 20, 2025

Uh oh!

llvmbot commented Aug 20, 2025

Uh oh!

llvmbot commented Aug 20, 2025

Uh oh!

kmclaughlin-arm commented Aug 20, 2025

Uh oh!

lukel97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-arm left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants