[PATCH] D88962: [SVE] Add support for scalable vectors in vectorize_width loop attribute

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.

LGTM with nits addressed. @fhahn are you happy with the changes?



================
Comment at: llvm/docs/LangRef.rst:5909
+   !0 = !{!"llvm.loop.vectorize.width", !1}
+   !1 = !{i32 4, i32 1}
 
----------------
nit: s/i32 1/i1 true/


================
Comment at: llvm/docs/LangRef.rst:5911
 
-Note that setting ``llvm.loop.vectorize.width`` to 1 disables
-vectorization of the loop. If ``llvm.loop.vectorize.width`` is set to
-0 or if the loop does not have this metadata the width will be
-determined automatically.
+where ``i32 4`` specifies the vector width and ``i32 1`` indicates if the
+vectorization factor is scalable, meaning that the loop-vectorizer should use
----------------
nit:
  s/vector width/minimum known vector width/
  s/and `i32 1`/and the non-zero value `i1 true`/


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:61
+  unsigned IntVal;
+  unsigned IsScalable = 0;
+
----------------
nit: `s/unsigned IsScalable = 0;/bool IsScalable = false;/`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:79
+
+  auto MaybeSetIntValue = [this](unsigned Val, bool Cond) {
+    if (Cond)
----------------
nit: `s/Maybe/Conditionally/`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:85
+
+  auto MaybeSetECValue = [this](unsigned Val, unsigned IsScalable, bool Cond) {
+    if (Cond)
----------------
nit: this is only used once, so better to inline.


================
Comment at: llvm/test/Transforms/LoopVectorize/metadata-width.ll:55
+!3 = !{!"llvm.loop.vectorize.width", !4}
+!4 = !{i32 8, i32 0}
----------------
nit: `s/i32 0/i1 false/`


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

https://reviews.llvm.org/D88962