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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks. Some small comments left inline.



================
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:
> 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.


================
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}
----------------
`-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`?


================
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
 
----------------
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.


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

https://reviews.llvm.org/D91718