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

preames · 2024-10-08T21:35:10Z

Previously, the cost model was returning an invalid cost. This simply moves the check from one place to another. This is mostly to make the cost modeling code a bit easier to follow.

… [nfc] Previously, the cost model was returning an invalid cost. This simply moves the check from one place to another. This is mostly to make the cost modeling code a bit easier to follow.

llvmbot · 2024-10-08T21:35:43Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-risc-v

Author: Philip Reames (preames)

Changes

Previously, the cost model was returning an invalid cost. This simply moves the check from one place to another. This is mostly to make the cost modeling code a bit easier to follow.

Full diff: https://github.com/llvm/llvm-project/pull/111592.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp (-2)
(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+7-1)

diff --git a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
index a61461681f79ed..34110b0f46107f 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
@@ -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
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 001c8987667df8..5427365aced175 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -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.
@@ -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)
+    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);

Mel-Chen

LGTM, thank you.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

fhahn · 2024-10-09T10:53:39Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+  // 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)


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?

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

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

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?

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.

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.

Co-authored-by: Mel Chen <[email protected]>

fhahn · 2024-10-09T18:11:21Z

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

@@ -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())


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

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.

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.

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.

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.

paulwalker-arm

For what it's worth this PR seems like a sensible refactor to me.

paulwalker-arm · 2024-10-14T16:17:03Z

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

@@ -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())


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.

fhahn

LGTM, thanks!

fhahn · 2024-10-14T19:22:39Z

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

@@ -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())


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.

… [nfc] (llvm#111592) Previously, the cost model was returning an invalid cost. This simply moves the check from one place to another. This is mostly to make the cost modeling code a bit easier to follow. --------- Co-authored-by: Mel Chen <[email protected]>

[LV] Check early for supported interleave factors with scalable types…

6514602

… [nfc] Previously, the cost model was returning an invalid cost. This simply moves the check from one place to another. This is mostly to make the cost modeling code a bit easier to follow.

preames requested review from fhahn and Mel-Chen October 8, 2024 21:35

llvmbot added backend:RISC-V vectorizers llvm:transforms labels Oct 8, 2024

Mel-Chen approved these changes Oct 9, 2024

View reviewed changes

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp Outdated Show resolved Hide resolved

fhahn reviewed Oct 9, 2024

View reviewed changes

preames and others added 2 commits October 9, 2024 09:44

Update llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

b8c097e

Co-authored-by: Mel Chen <[email protected]>

Update aarch64 cost model

3a5c127

llvmbot added the backend:AArch64 label Oct 9, 2024

fhahn reviewed Oct 9, 2024

View reviewed changes

paulwalker-arm approved these changes Oct 14, 2024

View reviewed changes

fhahn approved these changes Oct 14, 2024

View reviewed changes

preames merged commit b3c687b into llvm:main Oct 15, 2024
8 of 10 checks passed

preames deleted the lv-scalable-interleave-limit branch October 15, 2024 14:37

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

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

Uh oh!

Conversation

preames commented Oct 8, 2024

Uh oh!

llvmbot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mel-Chen Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulwalker-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Oct 8, 2024 •

edited

Loading

Mel-Chen Oct 9, 2024 •

edited

Loading