[PATCH] D98509: [LV] Calculate max feasible scalable VF.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:138
+    bool IsForcedScalable = (Scalable.Value == LoopVectorizeHints::FK_Enabled);
+    return Width.Value == 0 && IsForcedScalable;
+  }
----------------
fhahn wrote:
> david-arm wrote:
> > Hi @sdesmalen, to be honest I'm not sure I fully understand this. If the user has set the pragma:
> > 
> >   #pragma clang loop vectorize_width(4, scalable)
> > 
> > then haven't they also explicitly disabled fixed width? Maybe it's just the name of the function that confuses me a little, since from the user's perspective it feels like
> > 
> >   #pragma clang loop vectorize_width(4, scalable)
> > 
> > and
> > 
> >   #pragma clang loop vectorize_width(scalable)
> > 
> > are both disabling fixed width. I thought they were both hints that can be dropped by the compiler if necessary?
> Would it be possible to decouple the change to compute the max VFs from the ones adding a new option & tweaking the handling here?
I've removed this code for now, as I'll have a follow-up patch that adds an option to enable/disable scalable vectorization separately.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5631
+
+    assert(ElementCount::isKnownGT(UserVF, MaxSafeUserVF));
+
----------------
david-arm wrote:
> Isn't this always going to fire for cases where UserVF is scalable and we cannot vectorise reductions? For example, getMaxLegalScalableVF returns ElementCount::getScalable(0) in this case.
The statement above `if (ElementCount::isKnownLE(UserVF, MaxSafeUserVF)) return UserVF` should make sure that this assert is always true.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5674
+                                  MaxSafeScalableVF))
+    if (Max.isScalable())
+      LLVM_DEBUG(dbgs() << "LV: Found feasible scalable VF = " << Max << "\n");
----------------
david-arm wrote:
> Are you deliberately ignoring the scalable case for now because this will be addressed in a later patch?
Correct!


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-hint.ll:185-188
+; CHECK-DBG: Found feasible scalable VF = vscale x 2
+; CHECK-DBG: LV: Selecting VF: 4.
 ; CHECK-LABEL: @test4
+; CHECK: <4 x i32>
----------------
c-rhodes wrote:
> Should this select the feasible scalable VF? I suppose this is related to the issue @david-arm pointed out in `LoopVectorizationCostModel::computeFeasibleMaxVF`
I don't think it should do that by default. Personally I think it's best if the vectorizer just ignores hints that are not valid, and calculate a better answer instead, rather than blindly opting for something that seems "close enough", but may not be profitable.

That's akin to what happens for fixed-width vectors; if the UserVF is not legal (e.g. VF=16), and VF=8 would be legal, it will pick the most profitable VF <= 8, which isn't necessarily 8. Extending this to scalable vectors, we can argue that if `VF=vscale x 8` is not legal, it should consider all VFs (fixed and scalable) up to whatever is legal. We can then add an LV option to favour scalable VFs if we know the cost-model doesn't have all the information to make the right decision (although that's something to consider for another patch).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98509/new/

https://reviews.llvm.org/D98509