[LV] Fix MVE regression from #132190 #141736
Conversation
SamTebbs33
requested review from
MacDue,
NickGuy-Arm,
davemgreen,
david-arm,
gbossu,
huntergr-arm,
john-brawn-arm,
mikaelholmen and
sdesmalen-arm
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Sam Tebbs (SamTebbs33) ChangesRegister 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:
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;
|
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Could we get some tests for this case? That this regression wasn't caught indicates that we're missing coverage in this area. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good idea, done! This has also gotten the riscv tests back to what they were before merging #132190.
Good idea, done. |
sdesmalen-arm
left a comment
sdesmalen-arm
left a comment
There was a problem hiding this comment.
#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?
|
Thanks Sander, I've pushed up a commit that tries your suggested |
|
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
left a comment
fhahn
left a comment
There was a problem hiding this comment.
Thanks Sander, I've pushed up a commit that tries your suggested
VPWidenPointerInductionRecipeapproach 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).
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. |
SamTebbs33
force-pushed
the
fix-reg-pressure-regression
branch
from
760eb57 to
949b6bf
Compare
There was a problem hiding this comment.
I think in the long run it makes sense to treat this separately to MaximizeVectorBandwidth, perhaps through another target control. And in the longer-run adding them as part of the cost-model. In the mean time this fixes a number of performance regressions reported, some of which were quite large for MVE. I'd recommend getting this in to be back to where we were, performance-wise.
There was a problem hiding this comment.
Can you adjust this to be more aligned?
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
LGTM, thanks! Missed the earlier update. Left a few small suggestions to clean up the new test case a bit more.
There was a problem hiding this comment.
Would be good to clean this up before landing, the checks and extra blocks shouldn't be needed.
There was a problem hiding this comment.
Removed the blocks 👍
There was a problem hiding this comment.
| %in.addr.049 = phi ptr [ %incdec.ptr2, %for.body ], [ %in, %for.body.preheader ] | |
| %out.addr.048 = phi ptr [ %incdec.ptr34, %for.body ], [ %out, %for.body.preheader ] | |
| %ptr.iv.1 = phi ptr [ %in, %for.body.preheader ], [ %ptr.iv.1.next, %for.body ] | |
| %ptr.iv.2 = phi ptr [ %out, %for.body.preheader ], [ %ptr.iv.2 , %for.body ] |
for consistency with other tests.
There was a problem hiding this comment.
| source_filename = "<source>" |
There was a problem hiding this comment.
Yeah I don't think so, removed.
There was a problem hiding this comment.
might be more explicit to pass the target fetaures throug hte RUN line.
There was a problem hiding this comment.
nit: This is just a suggestion, but you could add an overloaded version of CM.useMaxBandwidth that takes an ElementCount and does something like:
bool useMaxBandwidth(ElementCount VF) {
return useMaxBandwidth(VF.isScalable() ? ...);
}
and use that here and in computeBestVF?
There was a problem hiding this comment.
I like that a lot, done!
Register pressure was only considered if the vector bandwidth was being maximised (chosen either by the target or user options), but llvm#132190 inadvertently caused high pressure VFs to be pruned even when max bandwidth wasn't enabled. This PR returns to the previous behaviour.
This reverts commit 760eb570768fa32f36dc64be73eb770bd27b691f.
SamTebbs33
force-pushed
the
fix-reg-pressure-regression
branch
from
50d2b87 to
ea866e8
Compare
Register pressure was only considered if the vector bandwidth was being maximised (chosen either by the target or user options), but llvm#132190 inadvertently caused high pressure VFs to be pruned even when max bandwidth wasn't enabled. This PR returns to the previous behaviour.
Stacked on #156923 In https://godbolt.org/z/8svWaredK, we spill a lot on RISC-V because whilst the largest element type is i8, we generate a bunch of pointer vectors for gathers and scatters. This means the VF chosen is quite high e.g. <vscale x 16 x i8>, but we end up using a bunch of <vscale x 16 x i64> m8 registers for the pointers. This was briefly fixed by #132190 where we computed register pressure in VPlan and used it to prune VFs that were likely to spill. The legacy cost model wasn't able to do this pruning because it didn't have visibility into the pointer vectors that were needed for the gathers/scatters. However VF pruning was restricted again to just the case when max bandwidth was enabled in #141736 to avoid an AArch64 regression, and restricted again in #149056 to only prune VFs that had max bandwidth enabled. On RISC-V we take advantage of register grouping for performance and choose a default of LMUL 2, which means there are 16 registers to work with – half the number as SVE, so we encounter higher register pressure more frequently. As such, we likely want to always consider pruning VFs with high register pressure and not just the VFs from max bandwidth. This adds a TTI hook to opt into this behaviour for RISC-V which fixes the motivating godbolt example above. When last checked this significantly reduces the number of spills on SPEC CPU 2017, up to 80% on 538.imagick_r.
8 participants
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.