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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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;
+      }
----------------
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)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5372
+    // Nothing to do if there are no dependencies.
+    if (MaxSafeRegisterWidth >= UINT_MAX)
+      return UserVF;
----------------
c-rhodes wrote:
> fhahn wrote:
> > This check seems a bit odd. I think we should at least use a named constant to make things clearer and ensure that LAA & LV are kept in sync on the meaning. But would it be possible to instead compute the maximum width of UserVF using MaxVScale (something like `UserVF * WidestType * MaxVScale`)?
> > This check seems a bit odd. I think we should at least use a named constant to make things clearer and ensure that LAA & LV are kept in sync on the meaning. But would it be possible to instead compute the maximum width of UserVF using MaxVScale (something like `UserVF * WidestType * MaxVScale`)?
> 
> I don't see how this is related, the reason I added this check is this block is validating a user VF and clamping it when it exceeds what is safe in terms of dependencies, but if there are no dependencies there's nothing to do here. I'm not sure if there's a better way of querying if there are no dependencies? `MaxSafeRegisterWidth` is initialized as `-1U` in LAA so I compared it against `UINT_MAX`, maybe it would be a little clearer if `MaxSafeRegisterWidth` was initialized as `UINT_MAX`. It would probably have made sense to introduced this in D90687 although I didn't consider it, it became obvious with this patch since large max safe VFs for loops with no dependencies were being emitted.
My point was that the original code handled the case where there are no dependencies naturally without an extra check, right? Is there a reason the logic for scalable vectors cannot do the same?


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

https://reviews.llvm.org/D91718