-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[TTI] Consistently pass the pointer type to getAddressComputationCost. NFCI #152657
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
lukel97
merged 6 commits into
llvm:main
from
lukel97:tti/getAddressComputationCost-ptr-ty
Aug 11, 2025
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8646bac
[TTI] Consistently pass the pointer type to getAddressComputationCost…
lukel97 c362c99
Use doxygen style
lukel97 eb3886c
Reuse Ptr
lukel97 24f9fc6
Merge branch 'main' into tti/getAddressComputationCost-ptr-ty
lukel97 a7a5181
Update doc comment
lukel97 1281755
Update comment
lukel97 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5293,11 +5293,12 @@ LoopVectorizationCostModel::getUniformMemOpCost(Instruction *I, | |
assert(Legal->isUniformMemOp(*I, VF)); | ||
|
||
Type *ValTy = getLoadStoreType(I); | ||
Type *PtrTy = getLoadStorePointerOperand(I)->getType(); | ||
auto *VectorTy = cast<VectorType>(toVectorTy(ValTy, VF)); | ||
const Align Alignment = getLoadStoreAlignment(I); | ||
unsigned AS = getLoadStoreAddressSpace(I); | ||
if (isa<LoadInst>(I)) { | ||
return TTI.getAddressComputationCost(ValTy) + | ||
return TTI.getAddressComputationCost(PtrTy) + | ||
TTI.getMemoryOpCost(Instruction::Load, ValTy, Alignment, AS, | ||
CostKind) + | ||
TTI.getShuffleCost(TargetTransformInfo::SK_Broadcast, VectorTy, | ||
|
@@ -5310,7 +5311,7 @@ LoopVectorizationCostModel::getUniformMemOpCost(Instruction *I, | |
// VF.getKnownMinValue() - 1 from a scalable vector. This does not represent | ||
// the actual generated code, which involves extracting the last element of | ||
// a scalable vector where the lane to extract is unknown at compile time. | ||
return TTI.getAddressComputationCost(ValTy) + | ||
return TTI.getAddressComputationCost(PtrTy) + | ||
TTI.getMemoryOpCost(Instruction::Store, ValTy, Alignment, AS, | ||
CostKind) + | ||
(IsLoopInvariantStoreValue | ||
|
@@ -5326,8 +5327,9 @@ LoopVectorizationCostModel::getGatherScatterCost(Instruction *I, | |
auto *VectorTy = cast<VectorType>(toVectorTy(ValTy, VF)); | ||
const Align Alignment = getLoadStoreAlignment(I); | ||
const Value *Ptr = getLoadStorePointerOperand(I); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you just reuse the
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woops yup, thanks |
||
Type *PtrTy = toVectorTy(Ptr->getType(), VF); | ||
|
||
return TTI.getAddressComputationCost(VectorTy) + | ||
return TTI.getAddressComputationCost(PtrTy) + | ||
TTI.getGatherScatterOpCost(I->getOpcode(), VectorTy, Ptr, | ||
Legal->isMaskRequired(I), Alignment, | ||
CostKind, I); | ||
|
@@ -5562,11 +5564,12 @@ LoopVectorizationCostModel::getMemoryInstructionCost(Instruction *I, | |
// moment. | ||
if (VF.isScalar()) { | ||
Type *ValTy = getLoadStoreType(I); | ||
Type *PtrTy = getLoadStorePointerOperand(I)->getType(); | ||
const Align Alignment = getLoadStoreAlignment(I); | ||
unsigned AS = getLoadStoreAddressSpace(I); | ||
|
||
TTI::OperandValueInfo OpInfo = TTI::getOperandInfo(I->getOperand(0)); | ||
return TTI.getAddressComputationCost(ValTy) + | ||
return TTI.getAddressComputationCost(PtrTy) + | ||
TTI.getMemoryOpCost(I->getOpcode(), ValTy, Alignment, AS, CostKind, | ||
OpInfo, I); | ||
} | ||
|
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're passing in the actual pointer type I thought it can only ever be an opaque pointer type, or a vector of opaque pointer types. So can we make the interface even simpler by just passing a boolean saying whether it's a vector or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the pointer type also contains the address space, not that any target currently uses it though. Long term though I think we probably want to change this API anyway to not even pass the type or SCEV? That way we can use it properly from VPlan which doesn't have access to SCEV. Just from looking at the various TTIs we seem to have three costs:
Maybe we could look at splitting this into three separate hooks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a single hook that takes an enum parameter providing more context? i.e.
where you'd have Stride == ConstantInt(X) for contiguous loads (X==1) or strided loads (X > 1), or Stride == non-constant for non-constant strides, or Stride == nullptr indicates these are gathers/scatters. Given that most uses of the SCEV argument seem to relate to the stride, perhaps that allows you to kill off the SCEV argument entirely this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah an enum would also work. I don't think we can directly pass a Value though if want to call this from the VPlan cost model where the value isn't materialized yet.
I presume this should be done in a separate PR anyway after we fix up the existing call sites to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure, although I made a typo as obviously
Value *Stride
is not an enum! :face_palm.