[LV] Fix MVE regression from #132190

[SamTebbs33]

Register pressure was only considered if the vector bandwidth was being maximised (chosen either by the target or user options), but #132190 inadvertently caused high pressure VFs to be pruned even when max bandwidth wasn't enabled. This PR returns to the previous behaviour.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Sam Tebbs (SamTebbs33)

Changes

Register pressure was only considered if the vector bandwidth was being maximised (chosen either by the target or user options), but #132190 inadvertently caused high pressure VFs to be pruned even when max bandwidth wasn't enabled. This PR returns to the previous behaviour.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+21-6)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 2fe59a464457f..ad3cbc6cd1e42 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -959,6 +959,10 @@ class LoopVectorizationCostModel {
     return expectedCost(UserVF).isValid();
   }
 
+  /// \return True if maximizing vector bandwidth is enabled by the target or
+  /// user options.
+  bool useMaxBandwidth(TargetTransformInfo::RegisterKind RegKind);
+
   /// \return The size (in bits) of the smallest and widest types in the code
   /// that needs to be vectorized. We ignore values that remain scalar such as
   /// 64 bit loop indices.
@@ -3944,6 +3948,14 @@ LoopVectorizationCostModel::computeMaxVF(ElementCount UserVF, unsigned UserIC) {
   return FixedScalableVFPair::getNone();
 }
 
+bool LoopVectorizationCostModel::useMaxBandwidth(
+    TargetTransformInfo::RegisterKind RegKind) {
+  return MaximizeBandwidth || (MaximizeBandwidth.getNumOccurrences() == 0 &&
+                               (TTI.shouldMaximizeVectorBandwidth(RegKind) ||
+                                (UseWiderVFIfCallVariantsPresent &&
+                                 Legal->hasVectorCallVariants())));
+}
+
 ElementCount LoopVectorizationCostModel::getMaximizedVFForTarget(
     unsigned MaxTripCount, unsigned SmallestType, unsigned WidestType,
     ElementCount MaxSafeVF, bool FoldTailByMasking) {
@@ -4009,10 +4021,7 @@ ElementCount LoopVectorizationCostModel::getMaximizedVFForTarget(
       ComputeScalableMaxVF ? TargetTransformInfo::RGK_ScalableVector
                            : TargetTransformInfo::RGK_FixedWidthVector;
   ElementCount MaxVF = MaxVectorElementCount;
-  if (MaximizeBandwidth ||
-      (MaximizeBandwidth.getNumOccurrences() == 0 &&
-       (TTI.shouldMaximizeVectorBandwidth(RegKind) ||
-        (UseWiderVFIfCallVariantsPresent && Legal->hasVectorCallVariants())))) {
+  if (useMaxBandwidth(RegKind)) {
     auto MaxVectorElementCountMaxBW = ElementCount::get(
         llvm::bit_floor(WidestRegister.getKnownMinValue() / SmallestType),
         ComputeScalableMaxVF);
@@ -4384,7 +4393,10 @@ VectorizationFactor LoopVectorizationPlanner::selectVectorizationFactor() {
 
       /// Don't consider the VF if it exceeds the number of registers for the
       /// target.
-      if (RU.exceedsMaxNumRegs(TTI))
+      if (CM.useMaxBandwidth(VF.isScalable()
+                                 ? TargetTransformInfo::RGK_ScalableVector
+                                 : TargetTransformInfo::RGK_FixedWidthVector) &&
+          RU.exceedsMaxNumRegs(TTI))
         continue;
 
       InstructionCost C = CM.expectedCost(VF);
@@ -7458,7 +7470,10 @@ VectorizationFactor LoopVectorizationPlanner::computeBestVF() {
       InstructionCost Cost = cost(*P, VF);
       VectorizationFactor CurrentFactor(VF, Cost, ScalarCost);
 
-      if (RU.exceedsMaxNumRegs(TTI)) {
+      if (CM.useMaxBandwidth(VF.isScalable()
+                                 ? TargetTransformInfo::RGK_ScalableVector
+                                 : TargetTransformInfo::RGK_FixedWidthVector) &&
+          RU.exceedsMaxNumRegs(TTI)) {
         LLVM_DEBUG(dbgs() << "LV(REG): Not considering vector loop of width "
                           << VF << " because it uses too many registers\n");
         continue;

[lukel97]

@preames @wangpc-pp @topperc Making a note here that after this patch we'll likely regress cases like this on RVV unless we opt into shouldMaximizeVectorBandwidth https://godbolt.org/z/WcbWGooa4

[NickGuy-Arm]

Could you explain this logic briefly? Maybe I'm misunderstanding the goal here, but if we exceed the number of registers but don't have maxBandwidth enabled, then we'd try to vectorize anyway? Does the call to CM.useMaxBandwidth want to be negated?

In other words; Why are we only considering whether we exceed the number of registers when maxBandwidth is enabled?

[SamTebbs33]

That understanding is correct. This was the behaviour before my previous PR accidentally changed it. I'll be looking into another sensible solution for this specific reproducer but this fixes the regression.

[NickGuy-Arm]

Could we get some tests for this case? That this regression wasn't caught indicates that we're missing coverage in this area.

[fhahn]

I think this is the only use of RU, would be good to skip the register pressure computation when it's not needed

Same below

[SamTebbs33]

Good idea, done! This has also gotten the riscv tests back to what they were before merging #132190.

[SamTebbs33]

Could we get some tests for this case? That this regression wasn't caught indicates that we're missing coverage in this area.

Good idea, done.

[sdesmalen-arm]

#132190 changed the behaviour such that VFs are pruned based on reg-usage regardless of whether maximize-vector-bandwidth is enabled. This means it now applies to more cases and exposes pre-existing bugs in the reg-usage computation. While I understand the rationale behind rolling back to previous behaviour, my concern is that this PR hides these bugs again rather than fixing them.

That being said, it could be that the registers pressure estimation is too coarse to be useful for the general case (i.e. non-maximized bandwidth VFs). If that is the consensus, then I think this PR should limit pruning to only those VFs that are added by maximize vector bandwidth.

For the test added in this PR, what I can see is that the reason the regusage is too high is because for a VF of 4 it tries to vectorise the stores (rather than scalarise them as happens for VF=2). This inserts a VPWidenPointerInductionRecipe for the pointer at the start of the block which is then used by the stores at the end of the block, increasing register pressure for vector registers. In practice, the code-generator will try to instantiate the widened (vector) induction pointer closer to its use, rather than at the start of the block. I think we can consider these values as 'scalar' (similar to what we do for e.g. VPReplicateRecipe) because they'll remain scalar until they have to be widened at the point of their use.

@SamTebbs33 and @fhah what do you think?

[SamTebbs33]

Thanks Sander, I've pushed up a commit that tries your suggested VPWidenPointerInductionRecipe approach and it does still vectorise, without the worry of causing RISC-V any regressions @lukel97