[TTI] getScalingFactorCost should return InstructionCost::getInvalid() instead of -1. #129802
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…) instead of -1. Historically this function return an int, with negative values meaning invalid. It was migrated to InstructionCost in 43ace8b, but the code was not updated to use invalid cost. In that commit, the caller in LSR was updated to assert that the cost is valid instead of positive.
llvmbot
added
backend:ARM
backend:AArch64
backend:X86
llvm:analysis
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-aarch64 Author: Craig Topper (topperc) ChangesHistorically this function return an int with negative values meaning invalid. It was migrated to InstructionCost in 43ace8b, but the code was not updated to return invalid cost. In that commit, the caller in LSR was updated to assert that the cost is valid instead of positive. Full diff: https://github.com/llvm/llvm-project/pull/129802.diff 8 Files Affected:
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index a8d6dd18266bb..99bd587b03b79 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -364,7 +364,7 @@ class TargetTransformInfoImplBase {
Scale, AddrSpace, /*I=*/nullptr,
BaseOffset.getScalable()))
return 0;
- return -1;
+ return InstructionCost::getInvalid();
}
bool LSRWithInstrQueries() const { return false; }
diff --git a/llvm/include/llvm/CodeGen/BasicTTIImpl.h b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
index 563953516a354..3f03c3acddfd1 100644
--- a/llvm/include/llvm/CodeGen/BasicTTIImpl.h
+++ b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
@@ -511,7 +511,7 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
AM.ScalableOffset = BaseOffset.getScalable();
if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace))
return 0;
- return -1;
+ return InstructionCost::getInvalid();
}
bool isTruncateFree(Type *Ty1, Type *Ty2) {
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
index 09a0e7ec172bf..ba019e1a4ecd5 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -5271,7 +5271,7 @@ AArch64TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// Scale represents reg2 * scale, thus account for 1 if
// it is not equal to 0 or 1.
return AM.Scale != 0 && AM.Scale != 1;
- return -1;
+ return InstructionCost::getInvalid();
}
bool AArch64TTIImpl::shouldTreatInstructionLikeSelect(const Instruction *I) {
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
index c7f8450213ae5..46765e7b22175 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
@@ -451,7 +451,7 @@ class AArch64TTIImpl : public BasicTTIImplBase<AArch64TTIImpl> {
/// mode represented by AM for this target, for a load/store
/// of the specified type.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, it returns a negative value.
+ /// If the AM is not supported, it returns an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
diff --git a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp b/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
index 6b3fa04798061..93a1316382fcc 100644
--- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
@@ -2669,7 +2669,7 @@ InstructionCost ARMTTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
return AM.Scale < 0 ? 1 : 0; // positive offsets execute faster
return 0;
}
- return -1;
+ return InstructionCost::getInvalid();
}
bool ARMTTIImpl::hasArmWideBranch(bool Thumb) const {
diff --git a/llvm/lib/Target/ARM/ARMTargetTransformInfo.h b/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
index 3a4f940088b2e..72bd98f548187 100644
--- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
+++ b/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
@@ -302,7 +302,7 @@ class ARMTTIImpl : public BasicTTIImplBase<ARMTTIImpl> {
/// getScalingFactorCost - Return the cost of the scaling used in
/// addressing mode represented by AM.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, the return value must be negative.
+ /// If the AM is not supported, the return value is an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
index f6552c500d7cf..fb0d6d1ffa414 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
@@ -7057,7 +7057,7 @@ InstructionCost X86TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// Scale represents reg2 * scale, thus account for 1
// as soon as we use a second register.
return AM.Scale != 0;
- return -1;
+ return InstructionCost::getInvalid();
}
InstructionCost X86TTIImpl::getBranchMispredictPenalty() const {
diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.h b/llvm/lib/Target/X86/X86TargetTransformInfo.h
index 7786616f89aa6..8fcaee0c7017f 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.h
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.h
@@ -254,7 +254,7 @@ class X86TTIImpl : public BasicTTIImplBase<X86TTIImpl> {
/// mode represented by AM for this target, for a load/store
/// of the specified type.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, it returns a negative value.
+ /// If the AM is not supported, it returns an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
|
|
@llvm/pr-subscribers-backend-arm Author: Craig Topper (topperc) ChangesHistorically this function return an int with negative values meaning invalid. It was migrated to InstructionCost in 43ace8b, but the code was not updated to return invalid cost. In that commit, the caller in LSR was updated to assert that the cost is valid instead of positive. Full diff: https://github.com/llvm/llvm-project/pull/129802.diff 8 Files Affected:
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index a8d6dd18266bb..99bd587b03b79 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -364,7 +364,7 @@ class TargetTransformInfoImplBase {
Scale, AddrSpace, /*I=*/nullptr,
BaseOffset.getScalable()))
return 0;
- return -1;
+ return InstructionCost::getInvalid();
}
bool LSRWithInstrQueries() const { return false; }
diff --git a/llvm/include/llvm/CodeGen/BasicTTIImpl.h b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
index 563953516a354..3f03c3acddfd1 100644
--- a/llvm/include/llvm/CodeGen/BasicTTIImpl.h
+++ b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
@@ -511,7 +511,7 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
AM.ScalableOffset = BaseOffset.getScalable();
if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace))
return 0;
- return -1;
+ return InstructionCost::getInvalid();
}
bool isTruncateFree(Type *Ty1, Type *Ty2) {
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
index 09a0e7ec172bf..ba019e1a4ecd5 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -5271,7 +5271,7 @@ AArch64TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// Scale represents reg2 * scale, thus account for 1 if
// it is not equal to 0 or 1.
return AM.Scale != 0 && AM.Scale != 1;
- return -1;
+ return InstructionCost::getInvalid();
}
bool AArch64TTIImpl::shouldTreatInstructionLikeSelect(const Instruction *I) {
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
index c7f8450213ae5..46765e7b22175 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
@@ -451,7 +451,7 @@ class AArch64TTIImpl : public BasicTTIImplBase<AArch64TTIImpl> {
/// mode represented by AM for this target, for a load/store
/// of the specified type.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, it returns a negative value.
+ /// If the AM is not supported, it returns an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
diff --git a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp b/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
index 6b3fa04798061..93a1316382fcc 100644
--- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
@@ -2669,7 +2669,7 @@ InstructionCost ARMTTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
return AM.Scale < 0 ? 1 : 0; // positive offsets execute faster
return 0;
}
- return -1;
+ return InstructionCost::getInvalid();
}
bool ARMTTIImpl::hasArmWideBranch(bool Thumb) const {
diff --git a/llvm/lib/Target/ARM/ARMTargetTransformInfo.h b/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
index 3a4f940088b2e..72bd98f548187 100644
--- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
+++ b/llvm/lib/Target/ARM/ARMTargetTransformInfo.h
@@ -302,7 +302,7 @@ class ARMTTIImpl : public BasicTTIImplBase<ARMTTIImpl> {
/// getScalingFactorCost - Return the cost of the scaling used in
/// addressing mode represented by AM.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, the return value must be negative.
+ /// If the AM is not supported, the return value is an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
index f6552c500d7cf..fb0d6d1ffa414 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
@@ -7057,7 +7057,7 @@ InstructionCost X86TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// Scale represents reg2 * scale, thus account for 1
// as soon as we use a second register.
return AM.Scale != 0;
- return -1;
+ return InstructionCost::getInvalid();
}
InstructionCost X86TTIImpl::getBranchMispredictPenalty() const {
diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.h b/llvm/lib/Target/X86/X86TargetTransformInfo.h
index 7786616f89aa6..8fcaee0c7017f 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.h
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.h
@@ -254,7 +254,7 @@ class X86TTIImpl : public BasicTTIImplBase<X86TTIImpl> {
/// mode represented by AM for this target, for a load/store
/// of the specified type.
/// If the AM is supported, the return value must be >= 0.
- /// If the AM is not supported, it returns a negative value.
+ /// If the AM is not supported, it returns an invalid cost.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
|
david-arm
reviewed
Mar 5, 2025
| @@ -364,7 +364,7 @@ class TargetTransformInfoImplBase { | |||
| Scale, AddrSpace, /*I=*/nullptr, | |||
| BaseOffset.getScalable())) | |||
| return 0; | |||
| return -1; | |||
| return InstructionCost::getInvalid(); | |||
There was a problem hiding this comment.
The fix looks sensible, but the fact all the tests pass means that we were also missing some test coverage. Do you know if it's even possible to expose the invalid cost path in a test?
There was a problem hiding this comment.
The only caller I've found so far is in getScalingFactorCost in LSR. If isAMCompletelyFolded returns true for an LSRUse::Address kind, TTI.getScalingFactorCost is called twice. Right after the calls there is an assert that the cost is valid. That used to be an assert that the cost is positive. You can see this on line 1782 of the diff to LoopStrengthReduce.cpp in 43ace8b
I think there is an implicit contract that if isAMCompletelyFolded returns true, then getScalingFactorCost should return a valid cost.
There was a problem hiding this comment.
Ah I see. That's fine then, and I guess since the LSR code asserts the cost is valid at least we should catch any problems.
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
…) instead of -1. (llvm#129802) Historically this function return an int with negative values meaning invalid. It was migrated to InstructionCost in 43ace8b, but the code was not updated to return invalid cost instead of -1. In that commit, the caller in LSR was updated to assert that the cost is valid instead of positive. We should return invalid instead of a negative value so LSR will assert if the cost isn't valid.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Historically this function return an int with negative values meaning invalid. It was migrated to InstructionCost in 43ace8b, but the code was not updated to return invalid cost instead of -1. In that commit, the caller in LSR was updated to assert that the cost is valid instead of positive. We should return invalid instead of a negative value so LSR will assert if the cost isn't valid.