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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5372
+    // Nothing to do if there are no dependencies.
+    if (MaxSafeRegisterWidth >= UINT_MAX)
+      return UserVF;
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5375
+
+    unsigned FixedVF = PowerOf2Floor(MaxSafeRegisterWidth / WidestType);
+    // If scalable, scale VF by vscale before checking if it's safe.
----------------
sdesmalen wrote:
> nit: `MaxSafeElements`?
> nit: `MaxSafeElements`?

Cheers that makes more sense, will update it


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5377
+    // If scalable, scale VF by vscale before checking if it's safe.
+    ElementCount MaxSafeVF =
+        UserVF.isScalable()
----------------
sdesmalen wrote:
> Instead of returning an Optional<ElementCount>, I'd prefer the code here to just clamp to MaxFixedVF,
> in this order:
> * first try to see if it can use the requested scalable VF.
> * if not, then try to see if it can use the requested VF with vscale = 1 (i.e. fixed width)
> * if not, then try if it can use a clamped fixed VF.
> Instead of returning an Optional<ElementCount>, I'd prefer the code here to just clamp to MaxFixedVF,
> in this order:
> * first try to see if it can use the requested scalable VF.
> * if not, then try to see if it can use the requested VF with vscale = 1 (i.e. fixed width)
> * if not, then try if it can use a clamped fixed VF.

Deferring to fixed-width so Optional isn't necessary is a good point, the first 2 cases should already be handled, I'll add the latter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91718