Skip to content

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

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.

Sign up for GitHub

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
merged 4 commits into from
Aug 26, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1e98a6
[LV] Return Invalid from getLegacyCost when instruction cost forced.
kmclaughlin-arm Aug 20, 2025
e9ccb54
- Replace getNumOccurrences() with ForceTargetInstructionCost
kmclaughlin-arm Aug 20, 2025
4b4d4a5
- Moved new test into force-target-instruction-cost.ll
kmclaughlin-arm Aug 20, 2025
2f14b65
- Use -force-target-instruction-cost=10
kmclaughlin-arm Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@david-arm david-arm Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

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

return Cost;
}

bool VPCostContext::isLegacyUniformAfterVectorization(Instruction *I,
Expand Down
24 changes: 24 additions & 0 deletions llvm/test/Transforms/LoopVectorize/AArch64/force-inst-cost-invalid.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Aug 20, 2025

Choose a reason for hiding this comment

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

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.

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
Copy link
Contributor

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Aug 20, 2025

Choose a reason for hiding this comment

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

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

entry:
br label %for.body

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

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Aug 20, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@fhahn fhahn Aug 21, 2025

Choose a reason for hiding this comment

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

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

%arrayidx = getelementptr ptr, ptr null, i64 %iv
Copy link
Contributor

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

please avoid storing to nullptr, with is UB.

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
Copy link
Contributor

@fhahn fhahn Aug 20, 2025

Choose a reason for hiding this comment

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

Suggested change
for.end:
ret i32 0
for.end:
ret void

}

attributes #0 = { "target-features"="+neon,+sve" vscale_range(1,16) }