[PATCH] D89031: [SVE] Add support to vectorize_width loop pragma for scalable vectors

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3034
+integer and the type of vectorization can be specified with an optional
+second parameter. In this case 'fixed' is the default and refers to fixed width
+vectorization, whereas 'scalable' indicates the compiler should use scalable
----------------
nit: s/In this case//


================
Comment at: clang/docs/LanguageExtensions.rst:3036
+vectorization, whereas 'scalable' indicates the compiler should use scalable
+vectors instead. In another variation of ``vectorize_width(fixed|scalable)``
+the user can hint at the type of vectorization to use without specifying
----------------
nit: Another use of vectorize_width is


================
Comment at: clang/docs/LanguageExtensions.rst:3038-3040
+the exact width. In both variants of the pragma if the target does not support
+scalable vectors then the vectorizer may decide to fall back on fixed width
+vectorization as the most profitable option.
----------------
nit: In both variants of the pragma the vectorizer may decide to fall back on fixed width vectorization if the target does not support scalable vectors.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1390
+def err_pragma_loop_invalid_vectorize_option : Error<
+  "vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') "
+  "where X is an integer, or vectorize_width('fixed' or 'scalable')">;
----------------
`use vectorize_width(X, fixed) or vectorize_width(X, scalable)`
(it may otherwise lead to confusion whether fixed/scalable needs quotes, same below)


================
Comment at: clang/lib/AST/AttrImpl.cpp:46
+  else if (state == FixedWidth || state == ScalableWidth) {
+    value->printPretty(OS, nullptr, Policy);
+    if (state == ScalableWidth)
----------------
is there always a value, even when "vectorize_width(scalable)" is specified?


================
Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:751-753
+          // they effectively want vectorization disabled. We leave the
+          // scalable flag unspecified in this case to avoid setting the
+          // vectorize.enable flag later on.
----------------
is that not something to fix in the code that conditionally sets vectorize.enable later on instead of working around it here?


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

https://reviews.llvm.org/D89031