[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

[davemgreen]

I haven't looked deeply into the details but there were a number of pretty large MVE regressions (and some improvements), only one of which was reported on #132190. MVE doesn't have a lot of registers but can deal with spills/reloads quite efficiently much of the time. Ideally it wouldn't be a strict yes/no but the cost of spilling would be included in the estimated cost of the vector factor.

There is another example in https://godbolt.org/z/Koeo86sYf, but it doesn't require any spilling. You can't really calculate register pressure (linearly) without scheduling the instructions in some way.

MVE doesn't enable maximise bandwidth. Would it be better to just have a target hook for whether regpressure should exclude a VF?

[fhahn]

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

I think unconditionally treating it as scalar will underestimate the vector register usage in some cases, and may introduce another set of regressions. It may be good to just limit this for maximizing vector bandwidth to start with, add test cases (would be great if @davemgreen could share any that are missing) and and fix/re-enable from a stable point. Overestimating the vector usage for VPWidenPointerInduction is problematic for the specific case, but there are other cases where we currently may overestimate (e.g. interleave groups).

[SamTebbs33]

I haven't looked deeply into the details but there were a number of pretty large MVE regressions (and some improvements), only one of which was reported on #132190. MVE doesn't have a lot of registers but can deal with spills/reloads quite efficiently much of the time. Ideally it wouldn't be a strict yes/no but the cost of spilling would be included in the estimated cost of the vector factor.

There is another example in https://godbolt.org/z/Koeo86sYf, but it doesn't require any spilling. You can't really calculate register pressure (linearly) without scheduling the instructions in some way.

MVE doesn't enable maximise bandwidth. Would it be better to just have a target hook for whether regpressure should exclude a VF?

Thanks Dave, I've had a look at the compiler explorer example you shared and it looks like there's no difference between current main, the "only check reg usage when max bandwidth is enabled" approach and the "treat VPWidenPointerInduction as a scalar" approach. So at least that one isn't regressing.

I'm in favour of Florian's suggestion to revert back to a stable point where register usage is only checked when maxbw is enabled and improve individual cases gradually, before perhaps enabling register usage checking globally once the regressing cases are sorted out.