[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:278
+    cl::desc(
+        "Allow scalable vectorization for targets that don't support scalable "
+        "vectors."));
----------------
It would be good to be clear in the message that this is just for testing (same as the previous option). Perhaps something like `Pretend that scalable vectors are supported, even if the target does not support them. The flag should only be used for testing.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5571
 
-  if (UserVF.isNonZero()) {
-    // For now, don't verify legality of scalable vectors.
-    // This will be addressed properly in https://reviews.llvm.org/D91718.
-    if (UserVF.isScalable())
+  bool IgnoreUserVF = UserVF.isScalable() && !TTI.supportsScalableVectors() &&
+                      !ForceTargetSupportsScalableVectors;
----------------
This is just about ignoring scalable UserVFs, right? Might be good to make that clear as part of the variable name.


================
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:
> c-rhodes wrote:
> > 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.
> If the test has a loop-carried distance that may invalidate auto-vectorization that needs checking, I think it's fair enough for the test to be moved to be a target-specific test (because that will require `getMaxVScale`).
> 
> For a test like metadata-width.ll it may be useful to add some `-force-target-supports-scalable-vectors=true` flag to avoid making all scalable-vector tests for the LoopVectorizer target-specific.
> 
> @fhahn do you have any strong feelings about this?
Yes that sounds reasonable to me.


================
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:
> > 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.
Ah right, if `getMaxVScale` can return `None`, then my suggestion doesn't work as expected. In that case, I think it would be good to encapsulate the check in LAA and have something like `areAllVectorWidthsSafe` or something. This should be more robust to future changes in LAA.


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

https://reviews.llvm.org/D91718