Skip to content

[LV] Check early for supported interleave factors with scalable types [nfc] #111592

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 3 commits into from
Oct 15, 2024
Merged

[LV] Check early for supported interleave factors with scalable types [nfc] #111592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6514602
[LV] Check early for supported interleave factors with scalable types…
preames Oct 8, 2024
b8c097e
Update llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
preames Oct 9, 2024
3a5c127
Update aarch64 cost model
preames Oct 9, 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
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 @@ -3743,7 +3743,7 @@ InstructionCost AArch64TTIImpl::getInterleavedMemoryOpCost(
assert(Factor >= 2 && "Invalid interleave factor");
auto *VecVTy = cast<VectorType>(VecTy);

if (VecTy->isScalableTy() && (!ST->hasSVE() || Factor != 2))
if (VecTy->isScalableTy() && !ST->hasSVE())
Copy link
Contributor

Choose a reason for hiding this comment

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

should this assert that Factor == 2 for SVE to guard against accidentally returning an incorrect non-invalid cost? Same for RISCV

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I can tell, the existing costs should be valid for Factor > 2. We already use them for fixed vector interleaves and for VF=2 scalable, so the code should be plausible. Once we add an IR representation for these (which we don't currently have), the costs will need audited, but the current implementation seems sane.

You seem reasonable opposed to what I consider a simple cleanup. I'm not particularly motivated to keep pushing this, if you'd rather I just drop the patch, please say so.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, I think all @fhahn is worried about here is a situation where the vectoriser may support larger interleave factors for scalable vectors but relies on the cost model returning Invalid for factors they don't support. So having an assert that Factor == 2 with scalable vectors for both RISC and AArch64 seems reasonable I guess?

The intent of this patch seems reasonable to me - by bailing out early in interleavedAccessCanBeWidened we can avoid doing unnecessary work. It's perhaps worth pointing that there is currently a patch (#89018) to add support for factors > 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm with @preames, there's no legal LLVM IR that cannot be handled by the code generator when scalable vectors are available and thus there's no reason for the cost model to return an invalid cost. #89018 changes nothing because it doesn't introduce any new IR and thus at worst the cost might be wrong, but it'll still safely make it through code generation.

The cost model shouldn't have to worry about non-existent LLVM IR, whereas it seems prudent for LoopVectorize to first check that what it's trying to do is possible within the current limitations of LLVM IR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking & clarifying @preames, just wanted to make sure to fully understand what's going on and make sure the costs returned should be reasonable when dropping the early exit.

return InstructionCost::getInvalid();

// Vectorization for masked interleaved accesses is only enabled for scalable
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,8 +675,6 @@ InstructionCost RISCVTTIImpl::getInterleavedMemoryOpCost(
unsigned Opcode, Type *VecTy, unsigned Factor, ArrayRef<unsigned> Indices,
Align Alignment, unsigned AddressSpace, TTI::TargetCostKind CostKind,
bool UseMaskForCond, bool UseMaskForGaps) {
if (isa<ScalableVectorType>(VecTy) && Factor != 2)
return InstructionCost::getInvalid();

// The interleaved memory access pass will lower interleaved memory ops (i.e
// a load and store followed by a specific shuffle) to vlseg/vsseg
Expand Down
8 changes: 7 additions & 1 deletion llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3406,6 +3406,7 @@ bool LoopVectorizationCostModel::interleavedAccessCanBeWidened(
"Decision should not be set yet.");
auto *Group = getInterleavedAccessGroup(I);
assert(Group && "Must have a group.");
unsigned InterleaveFactor = Group->getFactor();

// If the instruction's allocated size doesn't equal it's type size, it
// requires padding and will be scalarized.
Expand All @@ -3414,9 +3415,14 @@ bool LoopVectorizationCostModel::interleavedAccessCanBeWidened(
if (hasIrregularType(ScalarTy, DL))
return false;

// We currently only know how to emit interleave/deinterleave with
// Factor=2 for scalable vectors. This is purely an implementation
// limit.
if (VF.isScalable() && InterleaveFactor != 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this handled for AArch64 SVE? Does the change mean support for different factors must be added for all architectures with scalable vectors at the same time?

Copy link
Contributor

@Mel-Chen Mel-Chen Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

SVE also has a similar check in interleavedAccessCanBeWidened.
Indeed, it would be better to clean them up too if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's the same check just later in LV, could it be replaced by an assert?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SVE also has a similar check in interleavedAccessCanBeWidened.
Indeed, it would be better to clean them up too if possible.

I can't find this check except in the AArch64 cost model (now removed). What are you referring to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the change mean support for different factors must be added for all architectures with scalable vectors at the same time?

This is an IR representation issue. We have no way to represent in IR a interleave load or store at anything other than Factor=2 today. When we change that, yes, we will have to audit the cost models for each of the targets which support the new intrinsic. That seems pretty standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

SVE also has a similar check in interleavedAccessCanBeWidened.
Indeed, it would be better to clean them up too if possible.

I can't find this check except in the AArch64 cost model (now removed). What are you referring to?

My mistake. I mentioned the wrong function earlier; it should be getInterleavedMemoryOpCost rather than interleavedAccessCanBeWidened.
And commit 3a5c127 has already fixed it, thank you.

return false;

// If the group involves a non-integral pointer, we may not be able to
// losslessly cast all values to a common type.
unsigned InterleaveFactor = Group->getFactor();
bool ScalarNI = DL.isNonIntegralPointerType(ScalarTy);
for (unsigned Idx = 0; Idx < InterleaveFactor; Idx++) {
Instruction *Member = Group->getMember(Idx);
Expand Down
Loading