[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: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;
----------------
fhahn wrote:
> fhahn wrote:
> > This is just about ignoring scalable UserVFs, right? Might be good to make that clear as part of the variable name.
> This early exit could also be on the top of the function, before we compute the register widths & co and is not specific to scalable vectors, right? Might be good to move,  although the 'scalable ignored' message might also needs to move.
> This early exit could also be on the top of the function, before we compute the register widths & co and is not specific to scalable vectors, right? Might be good to move,  although the 'scalable ignored' message might also needs to move.

That's right it's not specific to scalable vectors, as long as `User.isNonZero()` it applies. I think you're right, if this is moved to the top `IgnoreScalableUserVF` will need to be part of the check, so:

```  // Nothing to do if there are no dependencies.
  if (UserVF.isNonZero() && !IgnoreScalableUserVF &&
      Legal->isSafeForAnyVectorWidth())
    return UserVF```

Otherwise scalable VFs will be accepted for targets without scalable vector support, where it currently falls into the existing code to pick a VF. It's fine with me to move this to the top with that in mind.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-loop-unpredicated-body-scalar-tail.ll:100
+!1 = distinct !{!1, !2, !3}
+!2 = !{!"llvm.loop.vectorize.width", i32 4}
+!3 = !{!"llvm.loop.vectorize.scalable.enable", i1 true}
----------------
fhahn wrote:
> `-force-vector-width` should also work, right? In that case, it would be good to use that at least for some tests.
> 
> Also, could this be kept target-independent with `-force-target-supports-scalable-vectors=true`?
> `-force-vector-width` should also work, right? In that case, it would be good to use that at least for some tests.

Yeah good idea, not sure we test the combination of that flag and the metadata, I'll change that.

> Also, could this be kept target-independent with `-force-target-supports-scalable-vectors=true`?

It can for this test since there's no dependencies so max vscale isn't required to determine if the scalable VF is valid. I'll implement your suggestion and keep this target-indepedent.



================
Comment at: llvm/test/Transforms/LoopVectorize/optimal-epilog-vectorization-limitations.ll:2
 ; REQUIRES: asserts
-; RUN: opt < %s  -passes='loop-vectorize' -force-vector-width=2 -enable-epilogue-vectorization -epilogue-vectorization-force-VF=2 --debug-only=loop-vectorize -S 2>&1 | FileCheck %s
+; RUN: opt < %s  -passes='loop-vectorize' -force-vector-width=2 -force-target-supports-scalable-vectors=true -enable-epilogue-vectorization -epilogue-vectorization-force-VF=2 --debug-only=loop-vectorize -S 2>&1 | FileCheck %s
 
----------------
fhahn wrote:
> is that necessary? I think it would be better to just add a separate test specifically checking that epilogue vectorization is disabled with scalable vectors.
> is that necessary? I think it would be better to just add a separate test specifically checking that epilogue vectorization is disabled with scalable vectors.

The final test here checks epilogue vectorization is disabled for scalable vectors, I can split that out but it'll still require this flag to target-independent. Or it could become an AArch64 test with SVE.


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

https://reviews.llvm.org/D91718