[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:
> 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?
> 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?

Sorry I missed your comment. One issue is for non-target specific tests with scalable VFs such as:
* Transforms/LoopVectorize/scalable-loop-unpredicated-body-scalar-tail.ll
* Transforms/LoopVectorize/metadata-width.ll
the hint will be ignored since scalable vectorization isn't supported, and also `getMaxVScale` now returns None. I suppose there's a couple of options, we could add new loop vectorizer flags or those tests could become become target-specific, I'm not sure what the best option is.


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

https://reviews.llvm.org/D91718