[PATCH] D91718: [LV] Legalize scalable VF hints

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 3 inline comments as done.
c-rhodes added inline comments.


================
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;
+      }
----------------
c-rhodes wrote:
> 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.
> > 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.

Done, although I've kept it as part of this patch. Ignoring the hint if scalable vectors aren't supported breaks existing tests so I've implemented the flag you suggested `-force-target-supports-scalable-vectors` which can be used for non-target specific tests. Although this only works for loops with no dependencies since `getMaxVScale` is None by default, so I've moved `scalable-loop-unpredicated-body-scalar-tail.ll` to AArch64 and enabled SVE.


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

https://reviews.llvm.org/D91718