[PATCH] D91718: [LV] Legalize scalable VF hints
Cullen Rhodes via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 11 06:23:08 PST 2020
c-rhodes added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5574
+ // Nothing to do if there are no dependencies.
+ if (MaxSafeVectorWidthInBits >= UINT_MAX)
return UserVF;
----------------
sdesmalen wrote:
> MaxSafeVectorWidthInBits can't exceed UINT_MAX. If this this early bail out isn't strictly necessary, I'd suggest removing it.
> MaxSafeVectorWidthInBits can't exceed UINT_MAX. If this this early bail out isn't strictly necessary, I'd suggest removing it.
There's a couple of existing tests:
* Transforms/LoopVectorize/scalable-loop-unpredicated-body-scalar-tail.ll
* Transforms/LoopVectorize/metadata-width.ll
that will fall over if the hint is ignored when scalable vectorization isn't supported. I suppose those could become target-specific tests.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5579-5585
+ if (!TTI.supportsScalableVectors()) {
+ reportVectorizationFailure(
+ "Ignoring scalable VF because target does not support scalable vectors",
+ "Ignoring scalable VF because target does not support scalable vectors.",
+ "NoScalableVectorSupport", ORE, TheLoop);
+ return None;
+ }
----------------
fhahn wrote:
> sdesmalen wrote:
> > Instead of returning 'None' (and thus not allowing any vectorziation), isn't it better to ignore the UserVF and continuing by allowing a VF to be chosen as normal?
> >
> > i.e.
> >
> > bool IgnoreUserVF = UserVF.isScalable() && TTI.supportsScalableVectors();
> > if (IgnoreUserVF)
> > ORE->emit([&]() {
> > return OptimizationRemark(....) };
> >
> > if (UserVF.isNonZero() && !IgnoreUserVF) {
> > ...
> > }
> >
> > // otherwise, let the LoopVectorizer choose a VF itself.
> >
> > I guess that can be split out into D93060 as well.
> Agreed, if the user requests scalable vectorization through a hint, I probably should just drop the hint and proceed with normal cost-modeling.
>
> (if there's a reason to keep the bail-out, can we just return a VF of 1? this is how other places in the function indicate that vectorization should be avoided)
> Instead of returning 'None' (and thus not allowing any vectorziation), isn't it better to ignore the UserVF and continuing by allowing a VF to be chosen as normal?
>
> ...
>
> I guess that can be split out into D93060 as well.
Ok np, I'll split that out then.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5604
+ // instead.
+ LLVM_DEBUG(
+ dbgs()
----------------
sdesmalen wrote:
> Should this be an OptRemark instead?
> Should this be an OptRemark instead?
Good point, I'll add that
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7433
- assert(!MaxVF.isScalable() &&
- "Scalable vectors not yet supported beyond this point");
+ if (MaxVF.isScalable()) {
+ // TODO: Use scalable MaxVF once we've added support for scalable
----------------
sdesmalen wrote:
> We still want to clamp to a smaller scalable VF if `UserVF.isScalable() && MaxVF.isScalable() && UserVF > MaxVF`. Because the loop below doesn't support scalable VFs, it is best to single that case out.
>
> That means:
> 1. UserVF = Scalable && MaxVF = Scalable && UserVF > MaxVF => Pick MaxVF
> 2. UserVF = Scalable && MaxVF = Fixed => Fall through into loop below.
> 3. UserVF = Fixed && MaxVF = Fixed && UserVF > MaxVF => Fall through into loop below.
>
> Where the clamping in 1. is temporary until the loop below works on scalable vectors, so that it can choose the most profitable VF based on the cost-model.
Ok, I'll add a temporary workaround for scalable VFs to select MaxVF above if UserVF is unsafe.
================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-hint.ll:1
+; REQUIRES: asserts
+; RUN: opt -mtriple=aarch64-none-linux-gnu -mattr=+sve -loop-vectorize -S < %s 2>&1 | FileCheck %s
----------------
sdesmalen wrote:
> Why does it require asserts?
> Why does it require asserts?
Because of the debug flags
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91718/new/
https://reviews.llvm.org/D91718
More information about the llvm-commits
mailing list