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 1 commit
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: 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

@fhahn fhahn Oct 9, 2024

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

@fhahn fhahn Oct 9, 2024

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

@preames preames Oct 9, 2024

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

@preames preames Oct 9, 2024

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

@Mel-Chen Mel-Chen Oct 10, 2024

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