Skip to content

[TTI] Support scalable offsets in getScalingFactorCost #88113

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
May 10, 2024
Merged

[TTI] Support scalable offsets in getScalingFactorCost #88113

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49ee8d7
[TTI] Support scalable offsets in getScalingFactorCost
huntergr-arm Apr 3, 2024
2a61c4d
Switch to StackOffset
huntergr-arm Apr 23, 2024
5228db4
Don't assert for scalable offsets for other targets
huntergr-arm May 8, 2024
10a454a
Remove unnecessary friend class declaration
huntergr-arm May 8, 2024
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
6 changes: 3 additions & 3 deletions llvm/include/llvm/Analysis/TargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ class TargetTransformInfo {
/// If the AM is not supported, it returns a negative value.
/// TODO: Handle pre/postinc as well.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we now have three approaches for passing a 'scalable' and a 'fixed-length' offset to interfaces for this:

  1. One using StackOffset (this patch for getScalingFactorCost)
  2. One passing in two separate variables (isLegalAddressingMode)
  3. Another one with two separate interfaces (isLegalAddImmediate vs isLegalAddScalableImmediate)

It would be good to settle on a single approach. @paulwalker-arm is there a particular reason you're pushing for (1) for this interface?

If we go for (1), I think we should rename StackOffset to MemOffset to make it more generic, because in this instance the offset does not necessarily apply only to stack addresses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made my request because the function was changed to take both a fixed-length and a scalar offset, which is exactly why StackOffset exists (albeit, as you say, with a name that's now out of date). I think all offset related common code interfaces should be unified to accept the fact they can be fixed or scaled and thus use whichever of the TypeSize.h family of types that best fits the specific need. I'll note we are missing a generic type like TypeSize but signed, to make this truly possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, my original patches did have a new type like that -- I originally called it AddressOffset, then TargetImmediate. I got some feedback that separating the terms out might be better, so we ended up with different int64_ts.

If we end up with an address formula with both fixed and scalable immediate terms, we'll have to prioritize one over the for a given target other due to the way LSR currently forms the Uses it will evaluate. AFAIK nobody implements instructions with mixed fixed and scalable immediates, so one or the other immediate would need to become part of the base for that formula.

int64_t Scale,
unsigned AddrSpace = 0) const;

Expand Down Expand Up @@ -1891,7 +1891,7 @@ class TargetTransformInfo::Concept {
virtual bool hasVolatileVariant(Instruction *I, unsigned AddrSpace) = 0;
virtual bool prefersVectorizedAddressing() = 0;
virtual InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset,
StackOffset BaseOffset,
bool HasBaseReg, int64_t Scale,
unsigned AddrSpace) = 0;
virtual bool LSRWithInstrQueries() = 0;
Expand Down Expand Up @@ -2403,7 +2403,7 @@ class TargetTransformInfo::Model final : public TargetTransformInfo::Concept {
return Impl.prefersVectorizedAddressing();
}
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale,
unsigned AddrSpace) override {
return Impl.getScalingFactorCost(Ty, BaseGV, BaseOffset, HasBaseReg, Scale,
Expand Down
9 changes: 6 additions & 3 deletions llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class Function;
/// Base class for use as a mix-in that aids implementing
/// a TargetTransformInfo-compatible class.
class TargetTransformInfoImplBase {
friend class TargetTransformInfo;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just wondered why this is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a leftover from a previous attempt at making an overloaded method to call the existing fixed-only method from the base implementation if there wasn't an override of the newer method. It didn't work out, so I've removed it. Thanks for spotting it.


protected:
typedef TargetTransformInfo TTI;

Expand Down Expand Up @@ -326,12 +328,13 @@ class TargetTransformInfoImplBase {
bool prefersVectorizedAddressing() const { return true; }

InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale,
unsigned AddrSpace) const {
// Guess that all legal addressing mode are free.
if (isLegalAddressingMode(Ty, BaseGV, BaseOffset, HasBaseReg, Scale,
AddrSpace))
if (isLegalAddressingMode(Ty, BaseGV, BaseOffset.getFixed(), HasBaseReg,
Scale, AddrSpace, /*I=*/nullptr,
BaseOffset.getScalable()))
return 0;
return -1;
}
Expand Down
5 changes: 3 additions & 2 deletions llvm/include/llvm/CodeGen/BasicTTIImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,14 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
}

InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) {
TargetLoweringBase::AddrMode AM;
AM.BaseGV = BaseGV;
AM.BaseOffs = BaseOffset;
AM.BaseOffs = BaseOffset.getFixed();
AM.HasBaseReg = HasBaseReg;
AM.Scale = Scale;
AM.ScalableOffset = BaseOffset.getScalable();
if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace))
return 0;
return -1;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Analysis/TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ bool TargetTransformInfo::prefersVectorizedAddressing() const {
}

InstructionCost TargetTransformInfo::getScalingFactorCost(
Type *Ty, GlobalValue *BaseGV, int64_t BaseOffset, bool HasBaseReg,
Type *Ty, GlobalValue *BaseGV, StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const {
InstructionCost Cost = TTIImpl->getScalingFactorCost(
Ty, BaseGV, BaseOffset, HasBaseReg, Scale, AddrSpace);
Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4162,7 +4162,7 @@ bool AArch64TTIImpl::preferPredicateOverEpilogue(TailFoldingInfo *TFI) {

InstructionCost
AArch64TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const {
// Scaling factors are not free at all.
// Operands | Rt Latency
Expand All @@ -4173,9 +4173,10 @@ AArch64TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// Rt, [Xn, Wm, <extend> #imm] |
TargetLoweringBase::AddrMode AM;
AM.BaseGV = BaseGV;
AM.BaseOffs = BaseOffset;
AM.BaseOffs = BaseOffset.getFixed();
AM.HasBaseReg = HasBaseReg;
AM.Scale = Scale;
AM.ScalableOffset = BaseOffset.getScalable();
if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace))
// Scale represents reg2 * scale, thus account for 1 if
// it is not equal to 0 or 1.
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 @@ -407,7 +407,7 @@ class AArch64TTIImpl : public BasicTTIImplBase<AArch64TTIImpl> {
/// If the AM is supported, the return value must be >= 0.
/// If the AM is not supported, it returns a negative value.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;
/// @}

Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2571,14 +2571,15 @@ bool ARMTTIImpl::preferPredicatedReductionSelect(
}

InstructionCost ARMTTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset,
StackOffset BaseOffset,
bool HasBaseReg, int64_t Scale,
unsigned AddrSpace) const {
TargetLoweringBase::AddrMode AM;
AM.BaseGV = BaseGV;
AM.BaseOffs = BaseOffset;
AM.BaseOffs = BaseOffset.getFixed();
AM.HasBaseReg = HasBaseReg;
AM.Scale = Scale;
assert(!BaseOffset.getScalable() && "Scalable offsets unsupported");
Copy link
Collaborator

@paulwalker-arm paulwalker-arm May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this assert is necessary. I'd just assign AM.ScalableOffset as normal. It's really up to the users of AM to assert ScalableOffset is zero. This is essentially what would need to happen is AM used the correct types to start within. In general though I'd expect most target to simply ignore the ScalableOffset field.

if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace)) {
if (ST->hasFPAO())
return AM.Scale < 0 ? 1 : 0; // positive offsets execute faster
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 @@ -303,7 +303,7 @@ class ARMTTIImpl : public BasicTTIImplBase<ARMTTIImpl> {
/// If the AM is supported, the return value must be >= 0.
/// If the AM is not supported, the return value must be negative.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;

bool maybeLoweredToCall(Instruction &I);
Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/Target/X86/X86TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6708,7 +6708,7 @@ InstructionCost X86TTIImpl::getInterleavedMemoryOpCost(
}

InstructionCost X86TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset,
StackOffset BaseOffset,
bool HasBaseReg, int64_t Scale,
unsigned AddrSpace) const {
// Scaling factors are not free at all.
Expand All @@ -6731,9 +6731,10 @@ InstructionCost X86TTIImpl::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
// vmovaps %ymm1, (%r8) can use port 2, 3, or 7.
TargetLoweringBase::AddrMode AM;
AM.BaseGV = BaseGV;
AM.BaseOffs = BaseOffset;
AM.BaseOffs = BaseOffset.getFixed();
AM.HasBaseReg = HasBaseReg;
AM.Scale = Scale;
assert(!BaseOffset.getScalable() && "Scalable offsets unsupported");
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, this is the responsibility of the users of AM.

if (getTLI()->isLegalAddressingMode(DL, AM, Ty, AddrSpace))
// Scale represents reg2 * scale, thus account for 1
// as soon as we use a second register.
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 @@ -253,7 +253,7 @@ class X86TTIImpl : public BasicTTIImplBase<X86TTIImpl> {
/// If the AM is supported, the return value must be >= 0.
/// If the AM is not supported, it returns a negative value.
InstructionCost getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
int64_t BaseOffset, bool HasBaseReg,
StackOffset BaseOffset, bool HasBaseReg,
int64_t Scale, unsigned AddrSpace) const;

bool isLSRCostLess(const TargetTransformInfo::LSRCost &C1,
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1817,10 +1817,12 @@ static InstructionCost getScalingFactorCost(const TargetTransformInfo &TTI,
case LSRUse::Address: {
// Check the scaling factor cost with both the min and max offsets.
InstructionCost ScaleCostMinOffset = TTI.getScalingFactorCost(
LU.AccessTy.MemTy, F.BaseGV, F.BaseOffset + LU.MinOffset, F.HasBaseReg,
LU.AccessTy.MemTy, F.BaseGV,
StackOffset::getFixed(F.BaseOffset + LU.MinOffset), F.HasBaseReg,
F.Scale, LU.AccessTy.AddrSpace);
InstructionCost ScaleCostMaxOffset = TTI.getScalingFactorCost(
LU.AccessTy.MemTy, F.BaseGV, F.BaseOffset + LU.MaxOffset, F.HasBaseReg,
LU.AccessTy.MemTy, F.BaseGV,
StackOffset::getFixed(F.BaseOffset + LU.MaxOffset), F.HasBaseReg,
F.Scale, LU.AccessTy.AddrSpace);

assert(ScaleCostMinOffset.isValid() && ScaleCostMaxOffset.isValid() &&
Expand Down
Loading