Skip to content

[CostModel] Remove some negative costs. #135533

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
merged 1 commit into from
Apr 28, 2025

Conversation

davemgreen
Copy link
Collaborator

The cost model in the past returned -1 for unknown costs, but over time this has largely been removed. This cleans up some of the uses that have remained. It uses 0/free for the cost of an insert and 1/basic for the cost of anything that is unknown.

@davemgreen davemgreen requested review from RKSimon and topperc April 13, 2025 08:50
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Apr 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 13, 2025

@llvm/pr-subscribers-llvm-analysis

Author: David Green (davemgreen)

Changes

The cost model in the past returned -1 for unknown costs, but over time this has largely been removed. This cleans up some of the uses that have remained. It uses 0/free for the cost of an insert and 1/basic for the cost of anything that is unknown.


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

2 Files Affected:

  • (modified) llvm/include/llvm/Analysis/TargetTransformInfoImpl.h (+5-5)
  • (modified) llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll (+6-6)
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index 3fe0a9101fdee..c9296edf5b81d 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -755,8 +755,9 @@ class TargetTransformInfoImplBase {
     // Note: The `insertvalue` cost here is chosen to match the default case of
     // getInstructionCost() -- as pior to adding this helper `insertvalue` was
     // not handled.
-    if (Opcode == Instruction::InsertValue)
-      return CostKind == TTI::TCK_RecipThroughput ? -1 : TTI::TCC_Basic;
+    if (Opcode == Instruction::InsertValue &&
+        CostKind != TTI::TCK_RecipThroughput)
+      return TTI::TCC_Basic;
     return TTI::TCC_Free;
   }
 
@@ -1578,9 +1579,8 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
     }
     }
 
-    // By default, just classify everything as 'basic' or -1 to represent that
-    // don't know the throughput cost.
-    return CostKind == TTI::TCK_RecipThroughput ? -1 : TTI::TCC_Basic;
+    // By default, just classify everything remaining as 'basic'.
+    return TTI::TCC_Basic;
   }
 
   bool isExpensiveToSpeculativelyExecute(const Instruction *I) {
diff --git a/llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll b/llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
index 25b066bb3dcbf..27c33dca5048a 100644
--- a/llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
+++ b/llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
@@ -58,8 +58,8 @@ define {<4 x i8>, <4 x i8>} @deinterleave_2(<8 x i8> %v) {
 ; CHECK-LABEL: 'deinterleave_2'
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 4 for instruction: %v0 = shufflevector <8 x i8> %v, <8 x i8> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 4 for instruction: %v1 = shufflevector <8 x i8> %v, <8 x i8> poison, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res0 = insertvalue { <4 x i8>, <4 x i8> } poison, <4 x i8> %v0, 0
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res1 = insertvalue { <4 x i8>, <4 x i8> } %res0, <4 x i8> %v1, 1
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res0 = insertvalue { <4 x i8>, <4 x i8> } poison, <4 x i8> %v0, 0
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res1 = insertvalue { <4 x i8>, <4 x i8> } %res0, <4 x i8> %v1, 1
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret { <4 x i8>, <4 x i8> } %res1
 ;
   %v0 = shufflevector <8 x i8> %v, <8 x i8> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
@@ -73,8 +73,8 @@ define {<4 x i32>, <4 x i32>} @deinterleave_2_m1_dest(<8 x i32> %v) {
 ; CHECK-LABEL: 'deinterleave_2_m1_dest'
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 8 for instruction: %v0 = shufflevector <8 x i32> %v, <8 x i32> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 8 for instruction: %v1 = shufflevector <8 x i32> %v, <8 x i32> poison, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res0 = insertvalue { <4 x i32>, <4 x i32> } poison, <4 x i32> %v0, 0
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res1 = insertvalue { <4 x i32>, <4 x i32> } %res0, <4 x i32> %v1, 1
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res0 = insertvalue { <4 x i32>, <4 x i32> } poison, <4 x i32> %v0, 0
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res1 = insertvalue { <4 x i32>, <4 x i32> } %res0, <4 x i32> %v1, 1
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret { <4 x i32>, <4 x i32> } %res1
 ;
   %v0 = shufflevector <8 x i32> %v, <8 x i32> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
@@ -88,8 +88,8 @@ define {<8 x i32>, <8 x i32>} @deinterleave_2_m2_dest(<16 x i32> %v) {
 ; CHECK-LABEL: 'deinterleave_2_m2_dest'
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 22 for instruction: %v0 = shufflevector <16 x i32> %v, <16 x i32> poison, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 22 for instruction: %v1 = shufflevector <16 x i32> %v, <16 x i32> poison, <8 x i32> <i32 1, i32 3, i32 5, i32 7, i32 9, i32 11, i32 13, i32 15>
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res0 = insertvalue { <8 x i32>, <8 x i32> } poison, <8 x i32> %v0, 0
-; CHECK-NEXT:  Cost Model: Found an estimated cost of -1 for instruction: %res1 = insertvalue { <8 x i32>, <8 x i32> } %res0, <8 x i32> %v1, 1
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res0 = insertvalue { <8 x i32>, <8 x i32> } poison, <8 x i32> %v0, 0
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %res1 = insertvalue { <8 x i32>, <8 x i32> } %res0, <8 x i32> %v1, 1
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret { <8 x i32>, <8 x i32> } %res1
 ;
   %v0 = shufflevector <16 x i32> %v, <16 x i32> poison, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

Why not use InstructionCost::getInvalid() ?

@davemgreen
Copy link
Collaborator Author

Why not use InstructionCost::getInvalid() ?

An Insert I would not expect to be invalid, and for the one at the end of getInstructionCost, if it can return a valid cost for !Recip returning the same cost as a fallback sounded OK to me. An Invalid cost to me means "this cannot be codegenerated" (or maybe just "strongly don't do this" at times), not "I don't know".

@davemgreen davemgreen force-pushed the gh-cm-dontuseminusone branch from 919ed2e to adce06f Compare April 21, 2025 05:59
@davemgreen davemgreen requested review from arsenm and lukel97 April 28, 2025 07:19
Copy link
Contributor

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

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

LGTM. Looks like those RISC-V insertvalues get lowered to COPYs which may or may not end up as vmv.v.vs, or otherwise a register rename.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

OK - LGTM (with a trivial minor)

@@ -755,8 +755,9 @@ class TargetTransformInfoImplBase {
// Note: The `insertvalue` cost here is chosen to match the default case of
// getInstructionCost() -- as pior to adding this helper `insertvalue` was
Copy link
Collaborator

Choose a reason for hiding this comment

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

pior -> prior

The cost model in the past returned -1 for unknown costs, but over time this
has largely been removed. This cleans up some of the uses that have remained.
It uses 0/free for the cost of an insert and 1/basic for the cost of anything
that is unknown.
@davemgreen davemgreen force-pushed the gh-cm-dontuseminusone branch from adce06f to c02e9be Compare April 28, 2025 17:38
@davemgreen davemgreen merged commit 86b7ce9 into llvm:main Apr 28, 2025
9 of 11 checks passed
@davemgreen davemgreen deleted the gh-cm-dontuseminusone branch April 28, 2025 20:08
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
The cost model in the past returned -1 for unknown costs, but over time
this has largely been removed. This cleans up some of the uses that have
remained. It uses 0/free for the cost of an insert and 1/basic for the
cost of anything that is unknown.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
The cost model in the past returned -1 for unknown costs, but over time
this has largely been removed. This cleans up some of the uses that have
remained. It uses 0/free for the cost of an insert and 1/basic for the
cost of anything that is unknown.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
The cost model in the past returned -1 for unknown costs, but over time
this has largely been removed. This cleans up some of the uses that have
remained. It uses 0/free for the cost of an insert and 1/basic for the
cost of anything that is unknown.
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this pull request May 7, 2025
The cost model in the past returned -1 for unknown costs, but over time
this has largely been removed. This cleans up some of the uses that have
remained. It uses 0/free for the cost of an insert and 1/basic for the
cost of anything that is unknown.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
The cost model in the past returned -1 for unknown costs, but over time
this has largely been removed. This cleans up some of the uses that have
remained. It uses 0/free for the cost of an insert and 1/basic for the
cost of anything that is unknown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants