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

[David Sherwood via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG38d18d93534d: [SVE] Add support to vectorize_width loop 
pragma for scalable vectors (authored by david-arm).

Changed prior to commit:
  https://reviews.llvm.org/D89031?vs=315121=315341#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/AST/AttrImpl.cpp
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp
  clang/test/Parser/pragma-loop.cpp

Index: clang/test/Parser/pragma-loop.cpp
===
--- clang/test/Parser/pragma-loop.cpp
+++ clang/test/Parser/pragma-loop.cpp
@@ -60,7 +60,8 @@
 
 template 
 void test_nontype_template_badarg(int *List, int Length) {
-  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I)
+  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I) /*
+ expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
   /* expected-error {{use of undeclared identifier 'Int'}} */ #pragma clang loop vectorize_width(V) interleave_count(Int)
   for (int i = 0; i < Length; i++) {
 List[i] = i;
@@ -74,6 +75,11 @@
   for (int i = 0; i < Length; i++) {
 List[i] = i;
   }
+
+  /* expected-error {{invalid value '-1'; must be positive}} */ #pragma clang loop vectorize_width(Value, fixed)
+  for (int i = 0; i < Length; i++) {
+List[i] = i;
+  }
 }
 
 void test(int *List, int Length) {
@@ -189,12 +195,15 @@
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1 +) 1
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1) +1
 const int VV = 4;
-/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2)
-/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined)
+/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
+/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(1+(^*/2 * ()
 /* expected-warning {{extra tokens at end of '#pragma clang loop' - ignored}} */ #pragma clang loop vectorize_width(1+(-0[0]))
 
-/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue)
+/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop interleave_count(badvalue)
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop unroll_count(badvalue)
   while (i-6 < Length) {
@@ -215,7 +224,7 @@
 /* expected-error {{invalid argument; expected 'enable', 'assume_safety' or 'disable'}} */ #pragma clang loop interleave(*)
 /* expected-error {{invalid argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll(=)
 /* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute(+)
-/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^)
+/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^) /* expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected expression}} expected-error {{expected expression}} */ #pragma clang loop interleave_count(/)
 /* 

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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for all the changes @david-arm!




Comment at: clang/lib/AST/AttrImpl.cpp:46
+  else if (state == FixedWidth || state == ScalableWidth) {
+if (value) {
+  value->printPretty(OS, nullptr, Policy);

nit:
```if (value) {
  value->printPretty(OS, nullptr, Policy);
  OS << ", ";
}
OS << (state == ScalableWidth ? "scalable" : "fixed";```
?



Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:309
+  (Attrs.VectorizeScalable == LoopAttributes::Disable &&
+   Attrs.VectorizeWidth == 0)) {
 bool AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;

nit: `!= 1` (it should be functionally the same because the >1 is already 
caught above, but this is specifically testing that VF=1 (scalar) is not 
specified)


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm updated this revision to Diff 315121.
david-arm added a comment.

- Updated documentation as per review comments.
- Fixed an issue with using value->prettyPrint on a null ptr.
- Reworked the code that sets vectorize.enable.


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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/AST/AttrImpl.cpp
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-print-pragmas.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp
  clang/test/Parser/pragma-loop.cpp

Index: clang/test/Parser/pragma-loop.cpp
===
--- clang/test/Parser/pragma-loop.cpp
+++ clang/test/Parser/pragma-loop.cpp
@@ -60,7 +60,8 @@
 
 template 
 void test_nontype_template_badarg(int *List, int Length) {
-  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I)
+  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I) /*
+ expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
   /* expected-error {{use of undeclared identifier 'Int'}} */ #pragma clang loop vectorize_width(V) interleave_count(Int)
   for (int i = 0; i < Length; i++) {
 List[i] = i;
@@ -189,12 +190,15 @@
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1 +) 1
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1) +1
 const int VV = 4;
-/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2)
-/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined)
+/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
+/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(1+(^*/2 * ()
 /* expected-warning {{extra tokens at end of '#pragma clang loop' - ignored}} */ #pragma clang loop vectorize_width(1+(-0[0]))
 
-/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue)
+/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop interleave_count(badvalue)
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop unroll_count(badvalue)
   while (i-6 < Length) {
@@ -215,7 +219,7 @@
 /* expected-error {{invalid argument; expected 'enable', 'assume_safety' or 'disable'}} */ #pragma clang loop interleave(*)
 /* expected-error {{invalid argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll(=)
 /* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute(+)
-/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^)
+/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^) /* expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, fixed) or vectorize_width(X, scalable) where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected expression}} expected-error {{expected expression}} */ #pragma clang loop interleave_count(/)
 /* expected-error {{expected expression}} expected-error {{expected expression}} */ #pragma clang loop unroll_count(==)
   while (i-8 < Length) {
Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,51 +158,97 @@
   

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

[David Sherwood via Phabricator via cfe-commits]

david-arm added inline comments.



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.

sdesmalen wrote:
> is that not something to fix in the code that conditionally sets 
> vectorize.enable later on instead of working around it here?
I did originally try to do that, but I had trouble with it and found it broke 
other places too. It ended up being simpler to fix it here, but I can play 
around with it again. Even if this is still the simplest solution I can come 
back with a more detailed explanation at least!


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.

LGTM, perhaps wait a day with committing in case there are more comments.




Comment at: clang/include/clang/Basic/Attr.td:3356
   EnumArgument<"State", "LoopHintState",
-   ["enable", "disable", "numeric", "assume_safety", 
"full"],
-   ["Enable", "Disable", "Numeric", "AssumeSafety", 
"Full"]>,
+   ["enable", "disable", "numeric", "fixed_width", 
"scalable_width", "assume_safety", "full"],
+   ["Enable", "Disable", "Numeric", "FixedWidth", 
"ScalableWidth", "AssumeSafety", "Full"]>,

aaron.ballman wrote:
> david-arm wrote:
> > aaron.ballman wrote:
> > > Should the documentation in AttrDocs.td be updated for this change?
> > Hi @aaron.ballman I had a look at LoopHintDocs in AttrDocs.td and it didn't 
> > explicitly mention these states, i.e. "assume_safety", "numeric", etc., so 
> > I'm not sure if it's necessary to add anything there?
> Oh, I see now, we're deferring to the documentation in the language 
> extensions document. I suppose that's fine as far as this patch goes, sorry 
> for the noise.
Nit: formatting, exceeding 80 columns?



Comment at: clang/include/clang/Basic/Attr.td:3357
+   ["enable", "disable", "numeric", "fixed_width", 
"scalable_width", "assume_safety", "full"],
+   ["Enable", "Disable", "Numeric", "FixedWidth", 
"ScalableWidth", "AssumeSafety", "Full"]>,
   ExprArgument<"Value">];

same?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Hi everyone, I realise that most people have probably been on holiday recently, 
but just a gentle ping here to see if anyone could take another look? Thanks!


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3356
   EnumArgument<"State", "LoopHintState",
-   ["enable", "disable", "numeric", "assume_safety", 
"full"],
-   ["Enable", "Disable", "Numeric", "AssumeSafety", 
"Full"]>,
+   ["enable", "disable", "numeric", "fixed_width", 
"scalable_width", "assume_safety", "full"],
+   ["Enable", "Disable", "Numeric", "FixedWidth", 
"ScalableWidth", "AssumeSafety", "Full"]>,

david-arm wrote:
> aaron.ballman wrote:
> > Should the documentation in AttrDocs.td be updated for this change?
> Hi @aaron.ballman I had a look at LoopHintDocs in AttrDocs.td and it didn't 
> explicitly mention these states, i.e. "assume_safety", "numeric", etc., so 
> I'm not sure if it's necessary to add anything there?
Oh, I see now, we're deferring to the documentation in the language extensions 
document. I suppose that's fine as far as this patch goes, sorry for the noise.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3356
   EnumArgument<"State", "LoopHintState",
-   ["enable", "disable", "numeric", "assume_safety", 
"full"],
-   ["Enable", "Disable", "Numeric", "AssumeSafety", 
"Full"]>,
+   ["enable", "disable", "numeric", "fixed_width", 
"scalable_width", "assume_safety", "full"],
+   ["Enable", "Disable", "Numeric", "FixedWidth", 
"ScalableWidth", "AssumeSafety", "Full"]>,

aaron.ballman wrote:
> Should the documentation in AttrDocs.td be updated for this change?
Hi @aaron.ballman I had a look at LoopHintDocs in AttrDocs.td and it didn't 
explicitly mention these states, i.e. "assume_safety", "numeric", etc., so I'm 
not sure if it's necessary to add anything there?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:3356
   EnumArgument<"State", "LoopHintState",
-   ["enable", "disable", "numeric", "assume_safety", 
"full"],
-   ["Enable", "Disable", "Numeric", "AssumeSafety", 
"Full"]>,
+   ["enable", "disable", "numeric", "fixed_width", 
"scalable_width", "assume_safety", "full"],
+   ["Enable", "Disable", "Numeric", "FixedWidth", 
"ScalableWidth", "AssumeSafety", "Full"]>,

Should the documentation in AttrDocs.td be updated for this change?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm updated this revision to Diff 312802.
david-arm edited the summary of this revision.
Herald added a subscriber: NickHung.

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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/AST/AttrImpl.cpp
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp
  clang/test/Parser/pragma-loop.cpp

Index: clang/test/Parser/pragma-loop.cpp
===
--- clang/test/Parser/pragma-loop.cpp
+++ clang/test/Parser/pragma-loop.cpp
@@ -60,7 +60,8 @@
 
 template 
 void test_nontype_template_badarg(int *List, int Length) {
-  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I)
+  /* expected-error {{use of undeclared identifier 'Vec'}} */ #pragma clang loop vectorize_width(Vec) interleave_count(I) /*
+ expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
   /* expected-error {{use of undeclared identifier 'Int'}} */ #pragma clang loop vectorize_width(V) interleave_count(Int)
   for (int i = 0; i < Length; i++) {
 List[i] = i;
@@ -189,12 +190,15 @@
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1 +) 1
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1) +1
 const int VV = 4;
-/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2)
-/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined)
+/* expected-error {{expected expression}} */ #pragma clang loop vectorize_width(VV +/ 2) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
+/* expected-error {{use of undeclared identifier 'undefined'}} */ #pragma clang loop vectorize_width(VV+undefined) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(1+(^*/2 * ()
 /* expected-warning {{extra tokens at end of '#pragma clang loop' - ignored}} */ #pragma clang loop vectorize_width(1+(-0[0]))
 
-/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue)
+/* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop vectorize_width(badvalue) /*
+   expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop interleave_count(badvalue)
 /* expected-error {{use of undeclared identifier 'badvalue'}} */ #pragma clang loop unroll_count(badvalue)
   while (i-6 < Length) {
@@ -215,7 +219,7 @@
 /* expected-error {{invalid argument; expected 'enable', 'assume_safety' or 'disable'}} */ #pragma clang loop interleave(*)
 /* expected-error {{invalid argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll(=)
 /* expected-error {{invalid argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute(+)
-/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^)
+/* expected-error {{type name requires a specifier or qualifier}} expected-error {{expected expression}} */ #pragma clang loop vectorize_width(^) /* expected-note {{vectorize_width loop hint malformed; use vectorize_width(X, 'fixed' or 'scalable') where X is an integer, or vectorize_width('fixed' or 'scalable')}} */
 /* expected-error {{expected expression}} expected-error {{expected expression}} */ #pragma clang loop interleave_count(/)
 /* expected-error {{expected expression}} expected-error {{expected expression}} */ #pragma clang loop unroll_count(==)
   while (i-8 < Length) {
Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,51 +158,88 @@
   for_template_constant_expression_test(List, Length);
 }
 
+// Verify for loop is performing fixed width vectorization
+void for_test_fixed_16(int *List, int Length) {
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) 

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks @david-arm for posting this proposal to the cfe list.
My confusion has been cleared up. The (new) proposal is to have:

1. vectorize_width(X) where X is an integer.
2. vectorize_width(X, fixed|scalable)
3. vectorize_width(fixed|scalable)

And with that 3rd option I agree that this allows us to express everything we 
want, and this patch needs adapting to this new proposal (just stating the 
obvious for clarity/completeness)

If scalable is used on a target that doesn't support this, a warning and 
falling back to fixed seems like the right thing to do.

I did have concerns about this, similarly like @fhahn:

> Hm, I am just a bit worried that it might be a bit confusing to users that do 
> not know what scalable vectors are (it is obvious when knowing all about SVE, 
> but I would assume most people don't necessarily know what this means). I 
> guess that is not the biggest deal, as long vectorize_width(X, scalable) 
> works for every target.

But since the new scalable option is opt-in, people don't need know about this 
if they don't want/need to, this should (hopefully) not be the case.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Because I was not understanding, we have discussed this further offline.

I think the conclusion was: pragma `vectorize_width` controls the vectorisation 
vector `VF` in ``. where `vscale` is not just a separate 
thing but it defines a VectorType. That's why it would make sense to attach 
`scalable|fixed` to `vectorize_width`. I agree with this and seems reasonable.

I still don't see how the proposed extension here allows you to specify fixed 
width vectorisation for:

  #pragma clang loop interleave_count(4)

targeting SVE and would appreciate if someone can comment on this example, but 
won't be holding up this work.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added a comment.

As I see it there are a bunch of pragmas that all enable vectorisation, with 
each pragma providing a unit of information.  One component of this information 
is the vectorisation factor hint provided by vectorize_width.

With the introduction of scalable vectors this hint is using the wrong datatype 
and thus needs to be updated to allow `vectorize_width(#num,[fixed|scalable])` 
and `vectorize_width([fixed|scalable])` along side the existing 
`vectorize_width(#num)` representation that effectively becomes an alias to 
`vectorize_width(#num, fixed)`.

Doing this means all existing usages work as expected and there's now extra 
power to better guide the chosen vectorisation factor.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> This approach should be fully complementary to `vectorize_with` so that it 
> would be possible to have:
>
>   // Use scalable vectors, but leave it to the cost-model to choose the most 
> efficient N in .
>   // If the pragma is not specified, it defaults to vectorize_style(fixed).
>   #pragma clang loop vectorize_style(scalable)
>   
>   // Use <4 x eltty>
>   #pragma clang loop vectorize_width(4, fixed)
>   
>   // Use 
>   #pragma clang loop vectorize_width(4, scalable)
>   
>   // If vectorize_style(scalable) is specified, then use  eltty>, otherwise <4 x eltty>
>   #pragma clang loop vectorize_width(4)   // uses <4 
> x eltty>
>   #pragma clang loop vectorize_width(4) vectorize_style(scalable) // uses 
> 
>   
>   // Conflicting options, clang should print diagnostic and error or ignore 
> the hint.
>   #pragma clang loop vectorize_width(4, fixed) vectorize_style(scalable)
>
> I hope that gives a bit more context.

Ok, thanks for clarifying that!

If:

  // Use 
  #pragma clang loop vectorize_width(4, scalable)

is equivalent to:

  // uses 
  #pragma clang loop vectorize_width(4) vectorize_style(scalable)

then I think that illustrates that I don't see the point of extending 
`vectorize_width` because we still can't express scalable vectorisation for:

  // 
  #pragma clang loop vectorize_predicate(enable)

and also for `interleave_count(4)`?

Again, when the idea is to have vectorize_style anyway, wouldn't it be easier 
not to bother extending vectorize_width and just go for vectorize_style? It 
allows you to specify fixed/scalable vectorisation in one way, and avoids 
having conflicting options.

The other thing I thought about: this is extending an existing user-facing 
pragma, and notifying the list would probably be best thing to do.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

In D89031#2391248 , @SjoerdMeijer 
wrote:

> In D89031#2391160 , @david-arm wrote:
>
>> Hi @SjoerdMeijer I think that given we now support scalable vectors we 
>> thought it made sense to be able to specify whether the user wants 'fixed' 
>> or 'scalable' vectorisation along with the vector width, although without 
>> specifying the additional property the default continues to remain 'fixed'. 
>> However, what you said about having a vectorize_scalable pragma is correct 
>> and we are intending to also add a pragma like this in a future patch.
>
> Okay, I haven't looked at the implementation to be honest, but am just trying 
> to understand the different use cases of this first.
> I just seem to be missing or not understanding why fixed/scalable is an 
> option to only vectorize_width, why not to vectorize(enable) or just a 
> separate one like vectorize_scalable? By making scalable/fixed and option to 
> vectorize_width, you can't toggle this for other pragmas like 
> interleave(enable) that enable vectorisation, which would be inconsistent? It 
> also seems to be more work to me to do this first for vectorize_width, and 
> then fix up other pragmas later. But I might be missing something (obivous) 
> here.

Hi @SjoerdMeijer, all valid and good questions. We think it makes sense to 
allow specifying explicitly what the meaning of '4' is when specifying the 
width. So that `vectorize_width(4, fixed)` means vectorizing with `<4 x eltty>` 
and `vectorize_width(4, scalable)` means vectorizing with ``. Like @david-arm said, we also plan to add something like 
`vectorize_style(fixed|scalable)`. This approach should be fully complementary 
to `vectorize_with` so that it would be possible to have:

  // Use scalable vectors, but leave it to the cost-model to choose the most 
efficient N in .
  // If the pragma is not specified, it defaults to vectorize_style(fixed).
  #pragma clang loop vectorize_style(scalable)
  
  // Use <4 x eltty>
  #pragma clang loop vectorize_width(4, fixed)
  
  // Use 
  #pragma clang loop vectorize_width(4, scalable)
  
  // If vectorize_style(scalable) is specified, then use , 
otherwise <4 x eltty>
  #pragma clang loop vectorize_width(4)   // uses <4 x 
eltty>
  #pragma clang loop vectorize_width(4) vectorize_style(scalable) // uses 

  
  // Conflicting options, clang should print diagnostic and error or ignore the 
hint.
  #pragma clang loop vectorize_width(4, fixed) vectorize_style(scalable)

I hope that gives a bit more context.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

In D89031#2391160 , @david-arm wrote:

> Hi @SjoerdMeijer I think that given we now support scalable vectors we 
> thought it made sense to be able to specify whether the user wants 'fixed' or 
> 'scalable' vectorisation along with the vector width, although without 
> specifying the additional property the default continues to remain 'fixed'. 
> However, what you said about having a vectorize_scalable pragma is correct 
> and we are intending to also add a pragma like this in a future patch.

Okay, I haven't looked at the implementation to be honest, but am just trying 
to understand the different use cases of this first.
I just seem to be missing or not understanding why fixed/scalable is an option 
to only vectorize_width, why not to vectorize(enable) or just a separate one 
like vectorize_scalable? By making scalable/fixed and option to 
vectorize_width, you can't toggle this for other pragmas like 
interleave(enable) that enable vectorisation, which would be inconsistent? It 
also seems to be more work to me to do this first for vectorize_width, and then 
fix up other pragmas later. But I might be missing something (obivous) here.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Hi @SjoerdMeijer I think that given we now support scalable vectors we thought 
it made sense to be able to specify whether the user wants 'fixed' or 
'scalable' vectorisation along with the vector width, although without 
specifying the additional property the default continues to remain 'fixed'. 
However, what you said about having a vectorize_scalable pragma is correct and 
we are intending to also add a pragma like this in a future patch.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I am very sorry that I am late to this... but I do have some concerns.

The concern that I have is that we extend vecorize_width with a scalable/fixed 
boolean, but there are more vectorisation pragma that set vectorisation options 
which imply enabling vectorisation:

> Pragmas setting transformation options imply the transformation is enabled, 
> as if it was enabled via the corresponding transformation pragma (e.g. 
> vectorize(enable))

Thus, unless I miss something, I don't think this should be an option to 
vectorize_width, but to me it looks like we need a separate one, e.g.:

  vectorize_scalable(enable|disable)

what do you think?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:3032
+The vector width is specified by
+``vectorize_width(_value_[, fixed|scalable])``, where __value__ is a positive
+integer and the type of vectorization can be specified with an optional

nit: `s/__value__/_value_/`


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm updated this revision to Diff 304488.

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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp
  clang/test/CodeGenCXX/pragma-scalable-loop.cpp

Index: clang/test/CodeGenCXX/pragma-scalable-loop.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pragma-scalable-loop.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature +sve -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// Verify do loop is performing scalable vectorization
+void for_test_scalable(int *List, int Length) {
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
+List[i] = i * 2;
+  }
+}
+
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_SCALABLE:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
+// CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[WIDTH_16_SCALABLE]] = !{!"llvm.loop.vectorize.width", ![[ELEMENT_COUNT_16_SCALABLE:.*]]}
+// CHECK: ![[ELEMENT_COUNT_16_SCALABLE]] = !{i32 16, i1 true}
+// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s 2>%t | FileCheck %s
+// RUN: FileCheck --check-prefix=CHECK-SCALABLE %s < %t
 
 // Verify while loop is recognized after sequence of pragma clang loop directives.
 void while_test(int *List, int Length) {
@@ -158,6 +159,26 @@
   for_template_constant_expression_test(List, Length);
 }
 
+// Verify for loop is performing fixed width vectorization
+void for_test_fixed(int *List, int Length) {
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+List[i] = i * 2;
+  }
+}
+
+// Verify for loop rejects scalable vectorization due to lack of target support
+// CHECK-SCALABLE: warning: the 'scalable' property of #pragma vectorize_width is unsupported by the target; assuming 'fixed' instead
+void for_test_scalable(int *List, int Length) {
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+// CHECK-SVE: br label {{.*}}, !llvm.loop ![[LOOP_16_SVE:.*]]
+List[i] = i * 2;
+  }
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
@@ -215,3 +236,8 @@
 
 // CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_FIXED]] = !{!"llvm.loop.vectorize.width", i32 16}
+
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -14,6 +14,7 @@
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ScopeInfo.h"
@@ -139,10 +140,21 @@
LoopHintAttr::PipelineInitiationInterval)
  .Case("distribute", LoopHintAttr::Distribute)
  .Default(LoopHintAttr::Vectorize);
-if (Option == LoopHintAttr::VectorizeWidth ||
-Option == 

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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

Hi @david-arm I just found that two uses of `llvm.loop.vectorize.width` are not 
yet updated.

- WarnMissedTransforms.cpp in `warnAboutLeftoverTransformations`.
- LoopUtils.cpp in `llvm::hasVectorizeTransformation`.

The cases seem quite trivial to fix up, can you include those changes in this 
patch?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

I'll hold off on any more changes for now to give @fhahn a chance to reply to 
your comment @sdesmalen about the fallback behaviour when scalable 
vectorisation is unsupported.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:939
+def warn_pragma_attribute_scalable_unused : Warning<
+  "ignoring scalable vectorize_width flag due to lack of target support">,
+  InGroup;

sdesmalen wrote:
> From what I can see, the vectorize_width flag is not ignored, only the 
> scalable property is. That means this should be:
>   'scalable' not supported by the target so assuming 'fixed' instead.
OK. I guess it's just when the warning comes out it appears at the start of the 
line so I wanted to emphasise that this relates to the scalable property passed 
to the vectorize_width attribute (rather than other attributes) as there could 
potentially be several pragmas on one line. I think it would be good to mention 
the vectorize_width pragma/attribute somewhere in the warning message to make 
it clear. I'll see if I can reword it.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/include/clang/Basic/DiagnosticSemaKinds.td:939
+def warn_pragma_attribute_scalable_unused : Warning<
+  "ignoring scalable vectorize_width flag due to lack of target support">,
+  InGroup;

From what I can see, the vectorize_width flag is not ignored, only the scalable 
property is. That means this should be:
  'scalable' not supported by the target so assuming 'fixed' instead.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:148
+  if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable")) {
+if (!S.Context.getTargetInfo().supportsScalableVectors()) {
+  S.Diag(St->getBeginLoc(), 
diag::warn_pragma_attribute_scalable_unused);

If the target does not support scalable vectors, it currently assumes 
`"fixed"`. If we want to stick with that approach, the diagnostic message 
should be changed (see my other comment). The alternative is dropping the hint 
entirely by returning `nullptr` and changing the diagnostic message to say the 
hint is ignored. I could live with both options. @fhahn do you have a 
preference here?

nit: to reduce nesting, can you hoist this out one level, e.g.
  if (StateLoc && StateLoc->Ident & ...)
State = LoopHintAttr::ScalableNumeric;
  else
State = LoopHintAttr::Numeric;

  if (State == LoopHintAttr::ScalableNumeric &&
  !S.Context.getTargetInfo().supportsScalableVectors()) {
S.Diag();
State = LoopHintAttr::Numeric;
  }


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm updated this revision to Diff 302773.
david-arm marked an inline comment as done.
david-arm edited the summary of this revision.

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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp
  clang/test/CodeGenCXX/pragma-scalable-loop.cpp

Index: clang/test/CodeGenCXX/pragma-scalable-loop.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pragma-scalable-loop.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature +sve -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// Verify do loop is performing scalable vectorization
+void for_test_scalable(int *List, int Length) {
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
+List[i] = i * 2;
+  }
+}
+
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_SCALABLE:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
+// CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[WIDTH_16_SCALABLE]] = !{!"llvm.loop.vectorize.width", ![[ELEMENT_COUNT_16_SCALABLE:.*]]}
+// CHECK: ![[ELEMENT_COUNT_16_SCALABLE]] = !{i32 16, i1 true}
+// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s 2>%t | FileCheck %s
+// RUN: FileCheck --check-prefix=CHECK-SCALABLE %s < %t
 
 // Verify while loop is recognized after sequence of pragma clang loop directives.
 void while_test(int *List, int Length) {
@@ -158,6 +159,26 @@
   for_template_constant_expression_test(List, Length);
 }
 
+// Verify for loop is performing fixed width vectorization
+void for_test_fixed(int *List, int Length) {
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+List[i] = i * 2;
+  }
+}
+
+// Verify for loop rejects scalable vectorization due to lack of target support
+// CHECK-SCALABLE: ignoring scalable vectorize_width flag due to lack of target support
+void for_test_scalable(int *List, int Length) {
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  for (int i = 0; i < Length; i++) {
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+// CHECK-SVE: br label {{.*}}, !llvm.loop ![[LOOP_16_SVE:.*]]
+List[i] = i * 2;
+  }
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
@@ -215,3 +236,8 @@
 
 // CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_FIXED]] = !{!"llvm.loop.vectorize.width", i32 16}
+
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -14,6 +14,7 @@
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ScopeInfo.h"
@@ -139,10 +140,21 @@
LoopHintAttr::PipelineInitiationInterval)
  .Case("distribute", LoopHintAttr::Distribute)
  .Default(LoopHintAttr::Vectorize);
-if (Option == LoopHintAttr::VectorizeWidth ||
- 

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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen requested changes to this revision.
sdesmalen added inline comments.
This revision now requires changes to proceed.



Comment at: clang/docs/LanguageExtensions.rst:3039
+useful for specifying the optimal width/count of the set of target
+architectures supported by your application.
 

Can you add a comment saying that the use of `"scalable"` is still experimental 
and is currently only intended to work for targets that support scalable 
vectors?



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;

fhahn wrote:
> sdesmalen wrote:
> > david-arm wrote:
> > > fhahn wrote:
> > > > Is there a way to only accept `fixed_width/scalable` for targets that 
> > > > support it? Not sure if we have enough information here, but we might 
> > > > be able to reject it eg per target basis or something
> > > Hi @fhahn, I think if possible we'd prefer not to reject scalable vectors 
> > > at this point. Theoretically there is no reason why we can't perform 
> > > scalable vectorisation for targets that don't have hardware support for 
> > > scalable vectors. In this case it simply means that vscale is 1. If you 
> > > want we could add some kind of opt-remark in the vectoriser that says 
> > > something like "target does not support scalable vectors, vectorising for 
> > > vscale=1"?
> > I agree with @david-arm that we shouldn't prevent this in the front-end. 
> > Even if the architecture may not support scalable vectors natively, there 
> > may still be reasons to want to create scalable vectors in IR, for example 
> > to have more portable IR.
> Hm, I am just a bit worried that it might be a bit confusing to users that do 
> not know what scalable vectors are (it is obvious when knowing all about SVE, 
> but I would assume most people don't necessarily know what this means). I 
> guess that is not the biggest deal, as long `vectorize_width(X, scalable)` 
> works for every target.
> 
> >  Even if the architecture may not support scalable vectors natively, there 
> > may still be reasons to want to create scalable vectors in IR, for example 
> > to have more portable IR.
> 
> Sure, but there are so many other target-specific things encoded that make 
> the IR really un-portable between targets. Granted, it is not impossible to 
> convert IR between some architectures (as in arm64_32)
Sorry, forgot to reply to this.

> Hm, I am just a bit worried that it might be a bit confusing to users that do 
> not know what scalable vectors are (it is obvious when knowing all about SVE, 
> but I would assume most people don't necessarily know what this means). I 
> guess that is not the biggest deal, as long vectorize_width(X, scalable) 
> works for every target.
At the moment this feature is still experimental, so I don't think any target 
would be able to return `true` to the question if this is supported :) That 
said, I agree that the compiler shouldn't crash for other targets after support 
in the loop-vectorizer stops being experimental. So I'm changing my mind here, 
and am happy to go with your suggestion to ignore the flag for other targets. 
When some default mechanism is added to lower scalable vectors to fixed-width 
vectors (for targets that don't natively support them), this check can probably 
be removed. @david-arm can you add some target hook to ignore the hint?

> Sure, but there are so many other target-specific things encoded that make 
> the IR really un-portable between targets. Granted, it is not impossible to 
> convert IR between some architectures (as in arm64_32)
I didn't mean portable between targets, but more as keeping the length of the 
vector agnostic in IR and leaving it until code-generation to pick a 
suitable/available vector extension, so that the same IR could be used to 
generate code for Neon or 256bit SVE for example. This is more a hypothetical 
use-case at the moment though.


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;

sdesmalen wrote:
> david-arm wrote:
> > fhahn wrote:
> > > Is there a way to only accept `fixed_width/scalable` for targets that 
> > > support it? Not sure if we have enough information here, but we might be 
> > > able to reject it eg per target basis or something
> > Hi @fhahn, I think if possible we'd prefer not to reject scalable vectors 
> > at this point. Theoretically there is no reason why we can't perform 
> > scalable vectorisation for targets that don't have hardware support for 
> > scalable vectors. In this case it simply means that vscale is 1. If you 
> > want we could add some kind of opt-remark in the vectoriser that says 
> > something like "target does not support scalable vectors, vectorising for 
> > vscale=1"?
> I agree with @david-arm that we shouldn't prevent this in the front-end. Even 
> if the architecture may not support scalable vectors natively, there may 
> still be reasons to want to create scalable vectors in IR, for example to 
> have more portable IR.
Hm, I am just a bit worried that it might be a bit confusing to users that do 
not know what scalable vectors are (it is obvious when knowing all about SVE, 
but I would assume most people don't necessarily know what this means). I guess 
that is not the biggest deal, as long `vectorize_width(X, scalable)` works for 
every target.

>  Even if the architecture may not support scalable vectors natively, there 
> may still be reasons to want to create scalable vectors in IR, for example to 
> have more portable IR.

Sure, but there are so many other target-specific things encoded that make the 
IR really un-portable between targets. Granted, it is not impossible to convert 
IR between some architectures (as in arm64_32)


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;

david-arm wrote:
> fhahn wrote:
> > Is there a way to only accept `fixed_width/scalable` for targets that 
> > support it? Not sure if we have enough information here, but we might be 
> > able to reject it eg per target basis or something
> Hi @fhahn, I think if possible we'd prefer not to reject scalable vectors at 
> this point. Theoretically there is no reason why we can't perform scalable 
> vectorisation for targets that don't have hardware support for scalable 
> vectors. In this case it simply means that vscale is 1. If you want we could 
> add some kind of opt-remark in the vectoriser that says something like 
> "target does not support scalable vectors, vectorising for vscale=1"?
I agree with @david-arm that we shouldn't prevent this in the front-end. Even 
if the architecture may not support scalable vectors natively, there may still 
be reasons to want to create scalable vectors in IR, for example to have more 
portable IR.



Comment at: clang/test/CodeGenCXX/pragma-loop.cpp:166
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) 
unroll(disable) distribute(disable)
+  do {
+// CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop 
![[LOOP_15:.*]]

out of curiosity, is there a particular reason you're testing it with a 
do-while loop instead of a shorter for-loop like the tests in 
`for_template_constant_expression_test` ?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm marked an inline comment as done.
david-arm added inline comments.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;

fhahn wrote:
> Is there a way to only accept `fixed_width/scalable` for targets that support 
> it? Not sure if we have enough information here, but we might be able to 
> reject it eg per target basis or something
Hi @fhahn, I think if possible we'd prefer not to reject scalable vectors at 
this point. Theoretically there is no reason why we can't perform scalable 
vectorisation for targets that don't have hardware support for scalable 
vectors. In this case it simply means that vscale is 1. If you want we could 
add some kind of opt-remark in the vectoriser that says something like "target 
does not support scalable vectors, vectorising for vscale=1"?


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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm updated this revision to Diff 299014.
david-arm added a comment.

- Rebase.


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

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,6 +158,30 @@
   for_template_constant_expression_test(List, Length);
 }
 
+// Verify do loop is performing fixed width vectorization
+void do_test_fixed(int *List, int Length) {
+  int i = 0;
+
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) unroll(disable) distribute(disable)
+  do {
+// CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+List[i] = i * 2;
+i++;
+  } while (i < Length);
+}
+
+// Verify do loop is performing scalable vectorization
+void do_test_scalable(int *List, int Length) {
+  int i = 0;
+
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  do {
+// CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+List[i] = i * 2;
+i++;
+  } while (i < Length);
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
@@ -215,3 +239,10 @@
 
 // CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_FIXED]] = !{!"llvm.loop.vectorize.width", i32 16}
+
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_SCALABLE:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_SCALABLE]] = !{!"llvm.loop.vectorize.width", ![[ELEMENT_COUNT_16_SCALABLE:.*]]}
+// CHECK: ![[ELEMENT_COUNT_16_SCALABLE]] = !{i32 16, i32 1}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -139,10 +139,17 @@
LoopHintAttr::PipelineInitiationInterval)
  .Case("distribute", LoopHintAttr::Distribute)
  .Default(LoopHintAttr::Vectorize);
-if (Option == LoopHintAttr::VectorizeWidth ||
-Option == LoopHintAttr::InterleaveCount ||
-Option == LoopHintAttr::UnrollCount ||
-Option == LoopHintAttr::PipelineInitiationInterval) {
+if (Option == LoopHintAttr::VectorizeWidth) {
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;
+  if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable"))
+State = LoopHintAttr::ScalableNumeric;
+  else
+State = LoopHintAttr::Numeric;
+} else if (Option == LoopHintAttr::InterleaveCount ||
+   Option == LoopHintAttr::UnrollCount ||
+   Option == LoopHintAttr::PipelineInitiationInterval) {
   assert(ValueExpr && "Attribute must have a valid value expression.");
   if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
 return nullptr;
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -1193,6 +1193,24 @@
 
 ExprResult R = ParseConstantExpression();
 
+if (OptionInfo && OptionInfo->getName() == "vectorize_width" &&
+Tok.is(tok::comma)) {
+  PP.Lex(Tok); // ,
+
+  SourceLocation StateLoc = Tok.getLocation();
+  IdentifierInfo *StateInfo = Tok.getIdentifierInfo();
+  StringRef IsScalableStr = StateInfo->getName();
+
+  if (IsScalableStr != "scalable" && IsScalableStr != "fixed") {
+Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_vectorize_option);
+return false;
+  }
+  PP.Lex(Tok); // Identifier
+
+  Hint.StateLoc =
+  IdentifierLoc::create(Actions.Context, StateLoc, StateInfo);
+}
+
 // Tokens following an error in an ill-formed constant expression will
 // remain in the token stream and must be removed.
 if (Tok.isNot(tok::eof)) {
Index: clang/lib/CodeGen/CGLoopInfo.h
===
--- clang/lib/CodeGen/CGLoopInfo.h
+++ clang/lib/CodeGen/CGLoopInfo.h
@@ -19,6 

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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.



Comment at: clang/lib/Parse/ParsePragma.cpp:1096
   static_cast(Tok.getAnnotationValue());
-
   IdentifierInfo *PragmaNameInfo = Info->PragmaName.getIdentifierInfo();

nit: unrelated change?



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;

Is there a way to only accept `fixed_width/scalable` for targets that support 
it? Not sure if we have enough information here, but we might be able to reject 
it eg per target basis or something


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89031

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Sherwood via Phabricator via cfe-commits]

david-arm created this revision.
david-arm added reviewers: sdesmalen, ctetreau, fhahn, c-rhodes.
Herald added subscribers: cfe-commits, psnobl, tschuett.
Herald added a reviewer: efriedma.
Herald added a reviewer: aaron.ballman.
Herald added a project: clang.
david-arm requested review of this revision.

This patch adds support for an optional second parameter passed to
the vectorize_width pragma, which indicates if the user wishes
to use fixed width or scalable vectorization. For example the user
can now write something like:

  #pragma clang loop vectorize_width(4, fixed)

or

  #pragma clang loop vectorize_width(4, scalable)

I have added a new 'scalable_numeric' state to the LoopHintAttr class
to indicate whether the numeric vectorization width is scalable or
not. When generating IR we make use of the new format for the
llvm.loop.vectorize.width attribute that allows us to effectively pass
an ElementCount that contains the vectorization factor and a scalable
flag.

Tests were added to

  clang/test/CodeGenCXX/pragma-loop.cpp

for both the 'fixed' and 'scalable' optional parameter.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89031

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGLoopInfo.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,6 +158,30 @@
   for_template_constant_expression_test(List, Length);
 }
 
+// Verify do loop is performing fixed width vectorization
+void do_test_fixed(int *List, int Length) {
+  int i = 0;
+
+#pragma clang loop vectorize_width(16, fixed) interleave_count(4) unroll(disable) distribute(disable)
+  do {
+// CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+List[i] = i * 2;
+i++;
+  } while (i < Length);
+}
+
+// Verify do loop is performing scalable vectorization
+void do_test_scalable(int *List, int Length) {
+  int i = 0;
+
+#pragma clang loop vectorize_width(16, scalable) interleave_count(4) unroll(disable) distribute(disable)
+  do {
+// CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+List[i] = i * 2;
+i++;
+  } while (i < Length);
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
@@ -215,3 +239,10 @@
 
 // CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_FIXED:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_FIXED]] = !{!"llvm.loop.vectorize.width", i32 16}
+
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_16_SCALABLE:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
+// CHECK: ![[WIDTH_16_SCALABLE]] = !{!"llvm.loop.vectorize.width", ![[ELEMENT_COUNT_16_SCALABLE:.*]]}
+// CHECK: ![[ELEMENT_COUNT_16_SCALABLE]] = !{i32 16, i32 1}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -139,10 +139,17 @@
LoopHintAttr::PipelineInitiationInterval)
  .Case("distribute", LoopHintAttr::Distribute)
  .Default(LoopHintAttr::Vectorize);
-if (Option == LoopHintAttr::VectorizeWidth ||
-Option == LoopHintAttr::InterleaveCount ||
-Option == LoopHintAttr::UnrollCount ||
-Option == LoopHintAttr::PipelineInitiationInterval) {
+if (Option == LoopHintAttr::VectorizeWidth) {
+  assert(ValueExpr && "Attribute must have a valid value expression.");
+  if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+return nullptr;
+  if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable"))
+State = LoopHintAttr::ScalableNumeric;
+  else
+State = LoopHintAttr::Numeric;
+} else if (Option == LoopHintAttr::InterleaveCount ||
+   Option == LoopHintAttr::UnrollCount ||
+   Option == LoopHintAttr::PipelineInitiationInterval) {
   assert(ValueExpr && "Attribute must have a valid value expression.");
   if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
 return nullptr;
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -1093,7 +1093,6 @@