Skip to content

[TTI] getScalingFactorCost should return InstructionCost::getInvalid() instead of -1. #129802

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 1 commit into from
Mar 5, 2025
Merged

[TTI] getScalingFactorCost should return InstructionCost::getInvalid() instead of -1. #129802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ class TargetTransformInfoImplBase {
Scale, AddrSpace, /*I=*/nullptr,
BaseOffset.getScalable()))
return 0;
return -1;
return InstructionCost::getInvalid();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

bool LSRWithInstrQueries() const { return false; }
Expand Down
2 changes: 1 addition & 1 deletion llvm/include/llvm/CodeGen/BasicTTIImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/ARM/ARMTargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/X86/X86TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/X86/X86TargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down