[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG89e61e782b73: [Sema][AArch64] Add semantics for 
arm_sve_vector_bits attribute (authored by c-rhodes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-arm-sve-vector-bits.c

Index: clang/test/Sema/attr-arm-sve-vector-bits.c
===
--- clang/test/Sema/attr-arm-sve-vector-bits.c
+++ clang/test/Sema/attr-arm-sve-vector-bits.c
@@ -60,3 +60,168 @@
 typedef float badtype3 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'float'}}
 typedef svint8x2_t badtype4 __attribute__((arm_sve_vector_bits(N)));// expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svint8x2_t' (aka '__clang_svint8x2_t')}}
 typedef svfloat32x3_t badtype5 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svfloat32x3_t' (aka '__clang_svfloat32x3_t')}}
+
+// Attribute only applies to typedefs.
+svint8_t non_typedef_type __attribute__((arm_sve_vector_bits(N)));  // expected-error {{'arm_sve_vector_bits' attribute only applies to typedefs}}
+
+// Test that we can define non-local fixed-length SVE types (unsupported for
+// sizeless types).
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;
+fixed_bool_t global_bool;
+
+extern fixed_int8_t extern_int8;
+extern fixed_bfloat16_t extern_bfloat16;
+extern fixed_bool_t extern_bool;
+
+static fixed_int8_t static_int8;
+static fixed_bfloat16_t static_bfloat16;
+static fixed_bool_t static_bool;
+
+fixed_int8_t *global_int8_ptr;
+extern fixed_int8_t *extern_int8_ptr;
+static fixed_int8_t *static_int8_ptr;
+__thread fixed_int8_t thread_int8;
+
+typedef fixed_int8_t int8_typedef;
+typedef fixed_int8_t *int8_ptr_typedef;
+
+// Test sized expressions
+int sizeof_int8 = sizeof(global_int8);
+int sizeof_int8_var = sizeof(*global_int8_ptr);
+int sizeof_int8_var_ptr = sizeof(global_int8_ptr);
+
+extern fixed_int8_t *extern_int8_ptr;
+
+int alignof_int8 = __alignof__(extern_int8);
+int alignof_int8_var = __alignof__(*extern_int8_ptr);
+int alignof_int8_var_ptr = __alignof__(extern_int8_ptr);
+
+void f(int c) {
+  fixed_int8_t fs8;
+  svint8_t ss8;
+
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
+}
+
+// --//
+// Sizeof
+
+#define VECTOR_SIZE ((N / 8))
+#define PRED_SIZE ((N / 64))
+
+_Static_assert(sizeof(fixed_int8_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_int16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_uint8_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_float16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bfloat16_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bool_t) == PRED_SIZE, "");
+
+// --//
+// Alignof
+
+#define VECTOR_ALIGN 16
+#define PRED_ALIGN 2
+
+_Static_assert(__alignof__(fixed_int8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_uint8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_float16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_float32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_float64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_bfloat16_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_bool_t) == PRED_ALIGN, "");
+
+// 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added a comment.

In D83551#2164984 , @aaron.ballman 
wrote:

> The attribute bits LGTM, thanks!


Thanks for reviewing!


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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

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

The attribute bits LGTM, thanks!




Comment at: clang/lib/AST/ASTContext.cpp:1887
+
+unsigned getSvePredWidth(const Type *T) { return getSveVectorWidth(T) / 8; }
+

c-rhodes wrote:
> aaron.ballman wrote:
> > Should this be dividing by the number of bits in a char for the target as 
> > opposed to hard-coding to 8?
> > Should this be dividing by the number of bits in a char for the target as 
> > opposed to hard-coding to 8?
> 
> Predicate registers in SVE hold one bit per byte of a vector register so each 
> predicate is 1/8th the size of a vector which are defined in bits, it has to 
> be 8 and I know `getCharWidth` returns 8 for the target this is implemented 
> for but I dont know what it would mean for any other target or if we care 
> about that?
Ah, so this is a case where we expect that value to always be 8 where this 
feature is supported anyway -- good to know. I still think the current code is 
a cleaner approach to hard-coding 8 bits, so thank you for making the change 
even though it doesn't enable much.


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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 279455.
c-rhodes added a comment.

- Make helpers static in ASTContext.
- Use `getCharWidth`.
- `s/vector-length-sized/vector-length-specific`.


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

https://reviews.llvm.org/D83551

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-arm-sve-vector-bits.c

Index: clang/test/Sema/attr-arm-sve-vector-bits.c
===
--- clang/test/Sema/attr-arm-sve-vector-bits.c
+++ clang/test/Sema/attr-arm-sve-vector-bits.c
@@ -60,3 +60,168 @@
 typedef float badtype3 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'float'}}
 typedef svint8x2_t badtype4 __attribute__((arm_sve_vector_bits(N)));// expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svint8x2_t' (aka '__clang_svint8x2_t')}}
 typedef svfloat32x3_t badtype5 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svfloat32x3_t' (aka '__clang_svfloat32x3_t')}}
+
+// Attribute only applies to typedefs.
+svint8_t non_typedef_type __attribute__((arm_sve_vector_bits(N)));  // expected-error {{'arm_sve_vector_bits' attribute only applies to typedefs}}
+
+// Test that we can define non-local fixed-length SVE types (unsupported for
+// sizeless types).
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;
+fixed_bool_t global_bool;
+
+extern fixed_int8_t extern_int8;
+extern fixed_bfloat16_t extern_bfloat16;
+extern fixed_bool_t extern_bool;
+
+static fixed_int8_t static_int8;
+static fixed_bfloat16_t static_bfloat16;
+static fixed_bool_t static_bool;
+
+fixed_int8_t *global_int8_ptr;
+extern fixed_int8_t *extern_int8_ptr;
+static fixed_int8_t *static_int8_ptr;
+__thread fixed_int8_t thread_int8;
+
+typedef fixed_int8_t int8_typedef;
+typedef fixed_int8_t *int8_ptr_typedef;
+
+// Test sized expressions
+int sizeof_int8 = sizeof(global_int8);
+int sizeof_int8_var = sizeof(*global_int8_ptr);
+int sizeof_int8_var_ptr = sizeof(global_int8_ptr);
+
+extern fixed_int8_t *extern_int8_ptr;
+
+int alignof_int8 = __alignof__(extern_int8);
+int alignof_int8_var = __alignof__(*extern_int8_ptr);
+int alignof_int8_var_ptr = __alignof__(extern_int8_ptr);
+
+void f(int c) {
+  fixed_int8_t fs8;
+  svint8_t ss8;
+
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
+}
+
+// --//
+// Sizeof
+
+#define VECTOR_SIZE ((N / 8))
+#define PRED_SIZE ((N / 64))
+
+_Static_assert(sizeof(fixed_int8_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_int16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_uint8_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_float16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bfloat16_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bool_t) == PRED_SIZE, "");
+
+// --//
+// Alignof
+
+#define VECTOR_ALIGN 16
+#define PRED_ALIGN 2
+
+_Static_assert(__alignof__(fixed_int8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_uint8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_float16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_float32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_float64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_bfloat16_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_bool_t) == PRED_ALIGN, "");
+
+// --//

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:1887
+
+unsigned getSvePredWidth(const Type *T) { return getSveVectorWidth(T) / 8; }
+

aaron.ballman wrote:
> Should this be dividing by the number of bits in a char for the target as 
> opposed to hard-coding to 8?
> Should this be dividing by the number of bits in a char for the target as 
> opposed to hard-coding to 8?

Predicate registers in SVE hold one bit per byte of a vector register so each 
predicate is 1/8th the size of a vector which are defined in bits, it has to be 
8 and I know `getCharWidth` returns 8 for the target this is implemented for 
but I dont know what it would mean for any other target or if we care about 
that?


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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I like the way the code is shaping up with the slightly altered design, nice!




Comment at: clang/lib/AST/ASTContext.cpp:1872
 
+unsigned getSveVectorWidth(const Type *T) {
+  // Get the vector size from the 'arm_sve_vector_bits' attribute via the

Should declare the method as `static`.



Comment at: clang/lib/AST/ASTContext.cpp:1887
+
+unsigned getSvePredWidth(const Type *T) { return getSveVectorWidth(T) / 8; }
+

Should this be dividing by the number of bits in a char for the target as 
opposed to hard-coding to 8?


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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

c-rhodes wrote:
> aaron.ballman wrote:
> > c-rhodes wrote:
> > > aaron.ballman wrote:
> > > > c-rhodes wrote:
> > > > > c-rhodes wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > c-rhodes wrote:
> > > > > > > > > sdesmalen wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > sdesmalen wrote:
> > > > > > > > > > > > nit: Can you add a comment saying why these are 
> > > > > > > > > > > > undocumented (and have no spellings)
> > > > > > > > > > > Also, I think these are all missing `let SemaHandler = 
> > > > > > > > > > > 0;` and `let ASTNode = 0;`
> > > > > > > > > > > 
> > > > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > > > instead of having a single type attribute which encodes 
> > > > > > > > > > > the N as an argument? I think this may simplify the patch 
> > > > > > > > > > > somewhat as you no longer need to switch over N as much.
> > > > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > > > instead of having a single type attribute which encodes 
> > > > > > > > > > > the N as an argument?
> > > > > > > > > > AIUI this was a workaround for getting the value of N from 
> > > > > > > > > > an AttributedType, because this only has `getAttrKind` to 
> > > > > > > > > > return the attribute kind, but no way to get the 
> > > > > > > > > > corresponding argument/value. This seemed like a simple way 
> > > > > > > > > > to do that without having to create a new subclass for Type 
> > > > > > > > > > and having to support that in various places. Is the latter 
> > > > > > > > > > the approach you were thinking of? (or is there perhaps a 
> > > > > > > > > > simpler way?)
> > > > > > > > > > Also, I think these are all missing let SemaHandler = 0; 
> > > > > > > > > > and let ASTNode = 0;
> > > > > > > > > 
> > > > > > > > > Good to know. In SemaType I'm doing `CurType = 
> > > > > > > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > > > > > > `AttributedType` in the AST, should I still set `let ASTNode 
> > > > > > > > > = 0;` in this case?
> > > > > > > > > 
> > > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > > instead of having a single type attribute which encodes the 
> > > > > > > > > > N as an argument?
> > > > > > > > > 
> > > > > > > > > As Sander mentioned, it seemed like the easiest solution, 
> > > > > > > > > interested to know if there's a better approach however
> > > > > > > > I was thinking specifically of creating a new `Type` subclass 
> > > > > > > > and supporting it rather than adding 5 new attributes that only 
> > > > > > > > vary by a bit-width (which means there's likely to be a 6th 
> > > > > > > > someday). It's not immediately clear to me whether that's a 
> > > > > > > > really big ask for little gain or not, though.
> > > > > > > Ah, you're right, we may still need `ASTNode` to be kept around 
> > > > > > > for that, good call.
> > > > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > > > ASTNode = 0;
> > > > > > 
> > > > > > I've added `let SemaHandler = 0;` for the internal types and `let 
> > > > > > ASTNode = 0;` for the user-facing attr.
> > > > > > I was thinking specifically of creating a new Type subclass and 
> > > > > > supporting it rather than adding 5 new attributes that only vary by 
> > > > > > a bit-width (which means there's likely to be a 6th someday).
> > > > > 
> > > > > It would be nice if the `Attr` was accessible from the 
> > > > > `AttributedType`, similar to how it is for `Decl`s, so something like:
> > > > > ```  if (const auto *AT = T->getAs())
> > > > > if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
> > > > >   unsigned Width = Attr->getNumBits();```
> > > > > Although I'm not sure if that makes sense or how easy it is. I do 
> > > > > agree adding 5 new attributes isn't ideal but for an initial 
> > > > > implementation it's nice and simple. Would you be ok with us 
> > > > > addressing this in a later patch?
> > > > > It would be nice if the Attr was accessible from the AttributedType, 
> > > > > similar to how it is for Decls, so something like:
> > > > 
> > > > You can do that through an `AttributedTypeLoc` object, which I think 
> > > > should be available from the situations you need to check this through 
> > > > a `TypeSourceInfo *` for the type. Then you can use 
> > > > `AttributedTypeLoc::getAttr()` to get the semantic attribute.
> > > > 
> > > > > Would you be ok with us addressing this in a later patch?
> > > > 
> > > > No and yes. It's a novel design to have a user-facing attribute that is 
> > > > never hooked up in the AST but is instead used to decide which 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 279178.
c-rhodes added a comment.

Changes:

- Remove internal type attributes (defined for each vector-size).
- Get the vector size from the `arm_sve_vector_bits` attribute via the 
`AttributedTypeLoc` associated with the typedef decl.
- Change `NumBits` argument for `ArmSveVectorBits` type attribute from int to 
unsigned.
- Only allow `ArmSveVectorBits` type attribute to be applied to typedefs (and 
added test).
- Set `let PragmaAttributeSupport = 0;` after specifying `Subjects` to fixed 
`clang/test/Misc/pragma-attribute-supported-attributes-list.test`.
- `vector-length sized` -> `vector-length-sized`.


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

https://reviews.llvm.org/D83551

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-arm-sve-vector-bits.c

Index: clang/test/Sema/attr-arm-sve-vector-bits.c
===
--- clang/test/Sema/attr-arm-sve-vector-bits.c
+++ clang/test/Sema/attr-arm-sve-vector-bits.c
@@ -60,3 +60,168 @@
 typedef float badtype3 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'float'}}
 typedef svint8x2_t badtype4 __attribute__((arm_sve_vector_bits(N)));// expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svint8x2_t' (aka '__clang_svint8x2_t')}}
 typedef svfloat32x3_t badtype5 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svfloat32x3_t' (aka '__clang_svfloat32x3_t')}}
+
+// Attribute only applies to typedefs.
+svint8_t non_typedef_type __attribute__((arm_sve_vector_bits(N)));  // expected-error {{'arm_sve_vector_bits' attribute only applies to typedefs}}
+
+// Test that we can define non-local fixed-length SVE types (unsupported for
+// sizeless types).
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;
+fixed_bool_t global_bool;
+
+extern fixed_int8_t extern_int8;
+extern fixed_bfloat16_t extern_bfloat16;
+extern fixed_bool_t extern_bool;
+
+static fixed_int8_t static_int8;
+static fixed_bfloat16_t static_bfloat16;
+static fixed_bool_t static_bool;
+
+fixed_int8_t *global_int8_ptr;
+extern fixed_int8_t *extern_int8_ptr;
+static fixed_int8_t *static_int8_ptr;
+__thread fixed_int8_t thread_int8;
+
+typedef fixed_int8_t int8_typedef;
+typedef fixed_int8_t *int8_ptr_typedef;
+
+// Test sized expressions
+int sizeof_int8 = sizeof(global_int8);
+int sizeof_int8_var = sizeof(*global_int8_ptr);
+int sizeof_int8_var_ptr = sizeof(global_int8_ptr);
+
+extern fixed_int8_t *extern_int8_ptr;
+
+int alignof_int8 = __alignof__(extern_int8);
+int alignof_int8_var = __alignof__(*extern_int8_ptr);
+int alignof_int8_var_ptr = __alignof__(extern_int8_ptr);
+
+void f(int c) {
+  fixed_int8_t fs8;
+  svint8_t ss8;
+
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
+}
+
+// --//
+// Sizeof
+
+#define VECTOR_SIZE ((N / 8))
+#define PRED_SIZE ((N / 64))
+
+_Static_assert(sizeof(fixed_int8_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_int16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_uint8_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_float16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bfloat16_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bool_t) == PRED_SIZE, "");
+
+// --//
+// Alignof
+
+#define VECTOR_ALIGN 16
+#define PRED_ALIGN 2
+
+_Static_assert(__alignof__(fixed_int8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_uint8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint32_t) == VECTOR_ALIGN, "");

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

aaron.ballman wrote:
> c-rhodes wrote:
> > aaron.ballman wrote:
> > > c-rhodes wrote:
> > > > c-rhodes wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > c-rhodes wrote:
> > > > > > > > sdesmalen wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > sdesmalen wrote:
> > > > > > > > > > > nit: Can you add a comment saying why these are 
> > > > > > > > > > > undocumented (and have no spellings)
> > > > > > > > > > Also, I think these are all missing `let SemaHandler = 0;` 
> > > > > > > > > > and `let ASTNode = 0;`
> > > > > > > > > > 
> > > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > > instead of having a single type attribute which encodes the 
> > > > > > > > > > N as an argument? I think this may simplify the patch 
> > > > > > > > > > somewhat as you no longer need to switch over N as much.
> > > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > > instead of having a single type attribute which encodes the 
> > > > > > > > > > N as an argument?
> > > > > > > > > AIUI this was a workaround for getting the value of N from an 
> > > > > > > > > AttributedType, because this only has `getAttrKind` to return 
> > > > > > > > > the attribute kind, but no way to get the corresponding 
> > > > > > > > > argument/value. This seemed like a simple way to do that 
> > > > > > > > > without having to create a new subclass for Type and having 
> > > > > > > > > to support that in various places. Is the latter the approach 
> > > > > > > > > you were thinking of? (or is there perhaps a simpler way?)
> > > > > > > > > Also, I think these are all missing let SemaHandler = 0; and 
> > > > > > > > > let ASTNode = 0;
> > > > > > > > 
> > > > > > > > Good to know. In SemaType I'm doing `CurType = 
> > > > > > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > > > > > `AttributedType` in the AST, should I still set `let ASTNode = 
> > > > > > > > 0;` in this case?
> > > > > > > > 
> > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > > as an argument?
> > > > > > > > 
> > > > > > > > As Sander mentioned, it seemed like the easiest solution, 
> > > > > > > > interested to know if there's a better approach however
> > > > > > > I was thinking specifically of creating a new `Type` subclass and 
> > > > > > > supporting it rather than adding 5 new attributes that only vary 
> > > > > > > by a bit-width (which means there's likely to be a 6th someday). 
> > > > > > > It's not immediately clear to me whether that's a really big ask 
> > > > > > > for little gain or not, though.
> > > > > > Ah, you're right, we may still need `ASTNode` to be kept around for 
> > > > > > that, good call.
> > > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > > ASTNode = 0;
> > > > > 
> > > > > I've added `let SemaHandler = 0;` for the internal types and `let 
> > > > > ASTNode = 0;` for the user-facing attr.
> > > > > I was thinking specifically of creating a new Type subclass and 
> > > > > supporting it rather than adding 5 new attributes that only vary by a 
> > > > > bit-width (which means there's likely to be a 6th someday).
> > > > 
> > > > It would be nice if the `Attr` was accessible from the 
> > > > `AttributedType`, similar to how it is for `Decl`s, so something like:
> > > > ```  if (const auto *AT = T->getAs())
> > > > if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
> > > >   unsigned Width = Attr->getNumBits();```
> > > > Although I'm not sure if that makes sense or how easy it is. I do agree 
> > > > adding 5 new attributes isn't ideal but for an initial implementation 
> > > > it's nice and simple. Would you be ok with us addressing this in a 
> > > > later patch?
> > > > It would be nice if the Attr was accessible from the AttributedType, 
> > > > similar to how it is for Decls, so something like:
> > > 
> > > You can do that through an `AttributedTypeLoc` object, which I think 
> > > should be available from the situations you need to check this through a 
> > > `TypeSourceInfo *` for the type. Then you can use 
> > > `AttributedTypeLoc::getAttr()` to get the semantic attribute.
> > > 
> > > > Would you be ok with us addressing this in a later patch?
> > > 
> > > No and yes. It's a novel design to have a user-facing attribute that is 
> > > never hooked up in the AST but is instead used to decide which 
> > > spellingless attribute to use instead, so I'd strongly prefer to find a 
> > > solution that doesn't use this approach. I also think we'll wind up with 
> > > cleaner code from this. That said, if it turns out to be awkward to do 
> > 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

c-rhodes wrote:
> aaron.ballman wrote:
> > c-rhodes wrote:
> > > c-rhodes wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > c-rhodes wrote:
> > > > > > > sdesmalen wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > sdesmalen wrote:
> > > > > > > > > > nit: Can you add a comment saying why these are 
> > > > > > > > > > undocumented (and have no spellings)
> > > > > > > > > Also, I think these are all missing `let SemaHandler = 0;` 
> > > > > > > > > and `let ASTNode = 0;`
> > > > > > > > > 
> > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > > as an argument? I think this may simplify the patch somewhat 
> > > > > > > > > as you no longer need to switch over N as much.
> > > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > > as an argument?
> > > > > > > > AIUI this was a workaround for getting the value of N from an 
> > > > > > > > AttributedType, because this only has `getAttrKind` to return 
> > > > > > > > the attribute kind, but no way to get the corresponding 
> > > > > > > > argument/value. This seemed like a simple way to do that 
> > > > > > > > without having to create a new subclass for Type and having to 
> > > > > > > > support that in various places. Is the latter the approach you 
> > > > > > > > were thinking of? (or is there perhaps a simpler way?)
> > > > > > > > Also, I think these are all missing let SemaHandler = 0; and 
> > > > > > > > let ASTNode = 0;
> > > > > > > 
> > > > > > > Good to know. In SemaType I'm doing `CurType = 
> > > > > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > > > > `AttributedType` in the AST, should I still set `let ASTNode = 
> > > > > > > 0;` in this case?
> > > > > > > 
> > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > as an argument?
> > > > > > > 
> > > > > > > As Sander mentioned, it seemed like the easiest solution, 
> > > > > > > interested to know if there's a better approach however
> > > > > > I was thinking specifically of creating a new `Type` subclass and 
> > > > > > supporting it rather than adding 5 new attributes that only vary by 
> > > > > > a bit-width (which means there's likely to be a 6th someday). It's 
> > > > > > not immediately clear to me whether that's a really big ask for 
> > > > > > little gain or not, though.
> > > > > Ah, you're right, we may still need `ASTNode` to be kept around for 
> > > > > that, good call.
> > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > ASTNode = 0;
> > > > 
> > > > I've added `let SemaHandler = 0;` for the internal types and `let 
> > > > ASTNode = 0;` for the user-facing attr.
> > > > I was thinking specifically of creating a new Type subclass and 
> > > > supporting it rather than adding 5 new attributes that only vary by a 
> > > > bit-width (which means there's likely to be a 6th someday).
> > > 
> > > It would be nice if the `Attr` was accessible from the `AttributedType`, 
> > > similar to how it is for `Decl`s, so something like:
> > > ```  if (const auto *AT = T->getAs())
> > > if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
> > >   unsigned Width = Attr->getNumBits();```
> > > Although I'm not sure if that makes sense or how easy it is. I do agree 
> > > adding 5 new attributes isn't ideal but for an initial implementation 
> > > it's nice and simple. Would you be ok with us addressing this in a later 
> > > patch?
> > > It would be nice if the Attr was accessible from the AttributedType, 
> > > similar to how it is for Decls, so something like:
> > 
> > You can do that through an `AttributedTypeLoc` object, which I think should 
> > be available from the situations you need to check this through a 
> > `TypeSourceInfo *` for the type. Then you can use 
> > `AttributedTypeLoc::getAttr()` to get the semantic attribute.
> > 
> > > Would you be ok with us addressing this in a later patch?
> > 
> > No and yes. It's a novel design to have a user-facing attribute that is 
> > never hooked up in the AST but is instead used to decide which spellingless 
> > attribute to use instead, so I'd strongly prefer to find a solution that 
> > doesn't use this approach. I also think we'll wind up with cleaner code 
> > from this. That said, if it turns out to be awkward to do because there's 
> > too much code required to support it, then I can likely hold my nose.
> > You can do that through an AttributedTypeLoc object, which I think should 
> > be available 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

aaron.ballman wrote:
> c-rhodes wrote:
> > c-rhodes wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > c-rhodes wrote:
> > > > > > sdesmalen wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > sdesmalen wrote:
> > > > > > > > > nit: Can you add a comment saying why these are undocumented 
> > > > > > > > > (and have no spellings)
> > > > > > > > Also, I think these are all missing `let SemaHandler = 0;` and 
> > > > > > > > `let ASTNode = 0;`
> > > > > > > > 
> > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > as an argument? I think this may simplify the patch somewhat as 
> > > > > > > > you no longer need to switch over N as much.
> > > > > > > > Is there a reason why we need N different type attributes 
> > > > > > > > instead of having a single type attribute which encodes the N 
> > > > > > > > as an argument?
> > > > > > > AIUI this was a workaround for getting the value of N from an 
> > > > > > > AttributedType, because this only has `getAttrKind` to return the 
> > > > > > > attribute kind, but no way to get the corresponding 
> > > > > > > argument/value. This seemed like a simple way to do that without 
> > > > > > > having to create a new subclass for Type and having to support 
> > > > > > > that in various places. Is the latter the approach you were 
> > > > > > > thinking of? (or is there perhaps a simpler way?)
> > > > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > > > ASTNode = 0;
> > > > > > 
> > > > > > Good to know. In SemaType I'm doing `CurType = 
> > > > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > > > `AttributedType` in the AST, should I still set `let ASTNode = 0;` 
> > > > > > in this case?
> > > > > > 
> > > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > > of having a single type attribute which encodes the N as an 
> > > > > > > argument?
> > > > > > 
> > > > > > As Sander mentioned, it seemed like the easiest solution, 
> > > > > > interested to know if there's a better approach however
> > > > > I was thinking specifically of creating a new `Type` subclass and 
> > > > > supporting it rather than adding 5 new attributes that only vary by a 
> > > > > bit-width (which means there's likely to be a 6th someday). It's not 
> > > > > immediately clear to me whether that's a really big ask for little 
> > > > > gain or not, though.
> > > > Ah, you're right, we may still need `ASTNode` to be kept around for 
> > > > that, good call.
> > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > ASTNode = 0;
> > > 
> > > I've added `let SemaHandler = 0;` for the internal types and `let ASTNode 
> > > = 0;` for the user-facing attr.
> > > I was thinking specifically of creating a new Type subclass and 
> > > supporting it rather than adding 5 new attributes that only vary by a 
> > > bit-width (which means there's likely to be a 6th someday).
> > 
> > It would be nice if the `Attr` was accessible from the `AttributedType`, 
> > similar to how it is for `Decl`s, so something like:
> > ```  if (const auto *AT = T->getAs())
> > if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
> >   unsigned Width = Attr->getNumBits();```
> > Although I'm not sure if that makes sense or how easy it is. I do agree 
> > adding 5 new attributes isn't ideal but for an initial implementation it's 
> > nice and simple. Would you be ok with us addressing this in a later patch?
> > It would be nice if the Attr was accessible from the AttributedType, 
> > similar to how it is for Decls, so something like:
> 
> You can do that through an `AttributedTypeLoc` object, which I think should 
> be available from the situations you need to check this through a 
> `TypeSourceInfo *` for the type. Then you can use 
> `AttributedTypeLoc::getAttr()` to get the semantic attribute.
> 
> > Would you be ok with us addressing this in a later patch?
> 
> No and yes. It's a novel design to have a user-facing attribute that is never 
> hooked up in the AST but is instead used to decide which spellingless 
> attribute to use instead, so I'd strongly prefer to find a solution that 
> doesn't use this approach. I also think we'll wind up with cleaner code from 
> this. That said, if it turns out to be awkward to do because there's too much 
> code required to support it, then I can likely hold my nose.
> You can do that through an AttributedTypeLoc object, which I think should be 
> available from the situations you need to check this through a TypeSourceInfo 
> * for the type. Then you can use AttributedTypeLoc::getAttr() to get the 
> semantic attribute.

I've tried your suggestion 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/Type.h:1928
 
+  /// Determines if this is vector-length sized typed (VLST), i.e. a
+  /// sizeless type with the 'arm_sve_vector_bits(N)' attribute applied.

... this is a vector-length-sized type...



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

c-rhodes wrote:
> c-rhodes wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > c-rhodes wrote:
> > > > > sdesmalen wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > sdesmalen wrote:
> > > > > > > > nit: Can you add a comment saying why these are undocumented 
> > > > > > > > (and have no spellings)
> > > > > > > Also, I think these are all missing `let SemaHandler = 0;` and 
> > > > > > > `let ASTNode = 0;`
> > > > > > > 
> > > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > > of having a single type attribute which encodes the N as an 
> > > > > > > argument? I think this may simplify the patch somewhat as you no 
> > > > > > > longer need to switch over N as much.
> > > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > > of having a single type attribute which encodes the N as an 
> > > > > > > argument?
> > > > > > AIUI this was a workaround for getting the value of N from an 
> > > > > > AttributedType, because this only has `getAttrKind` to return the 
> > > > > > attribute kind, but no way to get the corresponding argument/value. 
> > > > > > This seemed like a simple way to do that without having to create a 
> > > > > > new subclass for Type and having to support that in various places. 
> > > > > > Is the latter the approach you were thinking of? (or is there 
> > > > > > perhaps a simpler way?)
> > > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > > ASTNode = 0;
> > > > > 
> > > > > Good to know. In SemaType I'm doing `CurType = 
> > > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > > `AttributedType` in the AST, should I still set `let ASTNode = 0;` in 
> > > > > this case?
> > > > > 
> > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > of having a single type attribute which encodes the N as an 
> > > > > > argument?
> > > > > 
> > > > > As Sander mentioned, it seemed like the easiest solution, interested 
> > > > > to know if there's a better approach however
> > > > I was thinking specifically of creating a new `Type` subclass and 
> > > > supporting it rather than adding 5 new attributes that only vary by a 
> > > > bit-width (which means there's likely to be a 6th someday). It's not 
> > > > immediately clear to me whether that's a really big ask for little gain 
> > > > or not, though.
> > > Ah, you're right, we may still need `ASTNode` to be kept around for that, 
> > > good call.
> > > Also, I think these are all missing let SemaHandler = 0; and let ASTNode 
> > > = 0;
> > 
> > I've added `let SemaHandler = 0;` for the internal types and `let ASTNode = 
> > 0;` for the user-facing attr.
> > I was thinking specifically of creating a new Type subclass and supporting 
> > it rather than adding 5 new attributes that only vary by a bit-width (which 
> > means there's likely to be a 6th someday).
> 
> It would be nice if the `Attr` was accessible from the `AttributedType`, 
> similar to how it is for `Decl`s, so something like:
> ```  if (const auto *AT = T->getAs())
> if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
>   unsigned Width = Attr->getNumBits();```
> Although I'm not sure if that makes sense or how easy it is. I do agree 
> adding 5 new attributes isn't ideal but for an initial implementation it's 
> nice and simple. Would you be ok with us addressing this in a later patch?
> It would be nice if the Attr was accessible from the AttributedType, similar 
> to how it is for Decls, so something like:

You can do that through an `AttributedTypeLoc` object, which I think should be 
available from the situations you need to check this through a `TypeSourceInfo 
*` for the type. Then you can use `AttributedTypeLoc::getAttr()` to get the 
semantic attribute.

> Would you be ok with us addressing this in a later patch?

No and yes. It's a novel design to have a user-facing attribute that is never 
hooked up in the AST but is instead used to decide which spellingless attribute 
to use instead, so I'd strongly prefer to find a solution that doesn't use this 
approach. I also think we'll wind up with cleaner code from this. That said, if 
it turns out to be awkward to do because there's too much code required to 
support it, then I can likely hold my nose.


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

https://reviews.llvm.org/D83551



___
cfe-commits mailing list

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

c-rhodes wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > c-rhodes wrote:
> > > > sdesmalen wrote:
> > > > > aaron.ballman wrote:
> > > > > > sdesmalen wrote:
> > > > > > > nit: Can you add a comment saying why these are undocumented (and 
> > > > > > > have no spellings)
> > > > > > Also, I think these are all missing `let SemaHandler = 0;` and `let 
> > > > > > ASTNode = 0;`
> > > > > > 
> > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > of having a single type attribute which encodes the N as an 
> > > > > > argument? I think this may simplify the patch somewhat as you no 
> > > > > > longer need to switch over N as much.
> > > > > > Is there a reason why we need N different type attributes instead 
> > > > > > of having a single type attribute which encodes the N as an 
> > > > > > argument?
> > > > > AIUI this was a workaround for getting the value of N from an 
> > > > > AttributedType, because this only has `getAttrKind` to return the 
> > > > > attribute kind, but no way to get the corresponding argument/value. 
> > > > > This seemed like a simple way to do that without having to create a 
> > > > > new subclass for Type and having to support that in various places. 
> > > > > Is the latter the approach you were thinking of? (or is there perhaps 
> > > > > a simpler way?)
> > > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > > ASTNode = 0;
> > > > 
> > > > Good to know. In SemaType I'm doing `CurType = 
> > > > State.getAttributedType(A, CurType, CurType);` which gives an 
> > > > `AttributedType` in the AST, should I still set `let ASTNode = 0;` in 
> > > > this case?
> > > > 
> > > > > Is there a reason why we need N different type attributes instead of 
> > > > > having a single type attribute which encodes the N as an argument?
> > > > 
> > > > As Sander mentioned, it seemed like the easiest solution, interested to 
> > > > know if there's a better approach however
> > > I was thinking specifically of creating a new `Type` subclass and 
> > > supporting it rather than adding 5 new attributes that only vary by a 
> > > bit-width (which means there's likely to be a 6th someday). It's not 
> > > immediately clear to me whether that's a really big ask for little gain 
> > > or not, though.
> > Ah, you're right, we may still need `ASTNode` to be kept around for that, 
> > good call.
> > Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 
> > 0;
> 
> I've added `let SemaHandler = 0;` for the internal types and `let ASTNode = 
> 0;` for the user-facing attr.
> I was thinking specifically of creating a new Type subclass and supporting it 
> rather than adding 5 new attributes that only vary by a bit-width (which 
> means there's likely to be a 6th someday).

It would be nice if the `Attr` was accessible from the `AttributedType`, 
similar to how it is for `Decl`s, so something like:
```  if (const auto *AT = T->getAs())
if (ArmSveVectorBitsAttr *Attr = AT->getAttr())
  unsigned Width = Attr->getNumBits();```
Although I'm not sure if that makes sense or how easy it is. I do agree adding 
5 new attributes isn't ideal but for an initial implementation it's nice and 
simple. Would you be ok with us addressing this in a later patch?


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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked 6 inline comments as done.
c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

aaron.ballman wrote:
> aaron.ballman wrote:
> > c-rhodes wrote:
> > > sdesmalen wrote:
> > > > aaron.ballman wrote:
> > > > > sdesmalen wrote:
> > > > > > nit: Can you add a comment saying why these are undocumented (and 
> > > > > > have no spellings)
> > > > > Also, I think these are all missing `let SemaHandler = 0;` and `let 
> > > > > ASTNode = 0;`
> > > > > 
> > > > > Is there a reason why we need N different type attributes instead of 
> > > > > having a single type attribute which encodes the N as an argument? I 
> > > > > think this may simplify the patch somewhat as you no longer need to 
> > > > > switch over N as much.
> > > > > Is there a reason why we need N different type attributes instead of 
> > > > > having a single type attribute which encodes the N as an argument?
> > > > AIUI this was a workaround for getting the value of N from an 
> > > > AttributedType, because this only has `getAttrKind` to return the 
> > > > attribute kind, but no way to get the corresponding argument/value. 
> > > > This seemed like a simple way to do that without having to create a new 
> > > > subclass for Type and having to support that in various places. Is the 
> > > > latter the approach you were thinking of? (or is there perhaps a 
> > > > simpler way?)
> > > > Also, I think these are all missing let SemaHandler = 0; and let 
> > > > ASTNode = 0;
> > > 
> > > Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A, 
> > > CurType, CurType);` which gives an `AttributedType` in the AST, should I 
> > > still set `let ASTNode = 0;` in this case?
> > > 
> > > > Is there a reason why we need N different type attributes instead of 
> > > > having a single type attribute which encodes the N as an argument?
> > > 
> > > As Sander mentioned, it seemed like the easiest solution, interested to 
> > > know if there's a better approach however
> > I was thinking specifically of creating a new `Type` subclass and 
> > supporting it rather than adding 5 new attributes that only vary by a 
> > bit-width (which means there's likely to be a 6th someday). It's not 
> > immediately clear to me whether that's a really big ask for little gain or 
> > not, though.
> Ah, you're right, we may still need `ASTNode` to be kept around for that, 
> good call.
> Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 0;

I've added `let SemaHandler = 0;` for the internal types and `let ASTNode = 0;` 
for the user-facing attr.



Comment at: clang/lib/AST/ASTContext.cpp:1897
+
+bool ASTContext::getArmSveVectorBits(const Type *T, unsigned ) const {
+  if (!T->isVLST())

sdesmalen wrote:
> nit: I find this name a bit misleading, because I would expect the 
> (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename 
> this to `getBitwidthForAttributedSveType` ?
> nit: I find this name a bit misleading, because I would expect the 
> (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename 
> this to getBitwidthForAttributedSveType ?

You're right, poor choice of name, I've updated it



Comment at: clang/lib/Sema/SemaType.cpp:7754
 /// the ACLE, such as svint32_t and svbool_t.
-static void HandleArmSveVectorBitsTypeAttr(QualType ,
-   const ParsedAttr , Sema ) {
+static void HandleArmSveVectorBitsTypeAttr(TypeProcessingState ,
+   QualType ,

sdesmalen wrote:
> Unrelated changes?
> Unrelated changes?

`TypeProcessingState ` is required to create an attributed type and 
`Sema` is attached to this so I removed it from the interface. This is inline 
with other attributed type handlers. I pulled `S.Context` out as it's used 
frequently.



Comment at: clang/lib/Sema/SemaType.cpp:7839
+  default:
+llvm_unreachable("unsupported vector size!");
+  case 128:

sdesmalen wrote:
> If we only support power-of-two for now, we should only have an 
> `llvm_unreachable` if we prevent parsing any of the other widths (and give an 
> appropriate diagnostic saying those widths are not yet supported).
> If we only support power-of-two for now, we should only have an 
> llvm_unreachable if we prevent parsing any of the other widths (and give an 
> appropriate diagnostic saying those widths are not yet supported).

Now that we check `VecSize != S.getLangOpts().ArmSveVectorBits` above I think 
this is ok as this shouldn't be reachable with `-msve-vector-bits=` 
validating the vector size.



Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:80
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types 
('svint8_t' (aka 

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes updated this revision to Diff 278430.
c-rhodes added a comment.

Changes:

- Documented internal type attributes.
- Set `ASTNode = 0` on user-facing `ArmSveVectorBitsAttr` as the internal type 
attrs are used in the AST. Also removed the case for this from `TypePrinter`.
- `getSveVectorWidth` now returns an `unsigned`. Added an unreachable if `T` 
has no attrs.
- `s/getArmSveVectorBits/getBitwidthForAttributedSveType`. Also now returns an 
`unsigned` and asserts if `!T->isVLST()`.
- Add a few comments in test and reduced them a little so we dont tell all 
types for structs / unions etc.


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

https://reviews.llvm.org/D83551

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-arm-sve-vector-bits.c

Index: clang/test/Sema/attr-arm-sve-vector-bits.c
===
--- clang/test/Sema/attr-arm-sve-vector-bits.c
+++ clang/test/Sema/attr-arm-sve-vector-bits.c
@@ -60,3 +60,165 @@
 typedef float badtype3 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'float'}}
 typedef svint8x2_t badtype4 __attribute__((arm_sve_vector_bits(N)));// expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svint8x2_t' (aka '__clang_svint8x2_t')}}
 typedef svfloat32x3_t badtype5 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svfloat32x3_t' (aka '__clang_svfloat32x3_t')}}
+
+// Test that we can define non-local fixed-length SVE types (unsupported for
+// sizeless types).
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;
+fixed_bool_t global_bool;
+
+extern fixed_int8_t extern_int8;
+extern fixed_bfloat16_t extern_bfloat16;
+extern fixed_bool_t extern_bool;
+
+static fixed_int8_t static_int8;
+static fixed_bfloat16_t static_bfloat16;
+static fixed_bool_t static_bool;
+
+fixed_int8_t *global_int8_ptr;
+extern fixed_int8_t *extern_int8_ptr;
+static fixed_int8_t *static_int8_ptr;
+__thread fixed_int8_t thread_int8;
+
+typedef fixed_int8_t int8_typedef;
+typedef fixed_int8_t *int8_ptr_typedef;
+
+// Test sized expressions
+int sizeof_int8 = sizeof(global_int8);
+int sizeof_int8_var = sizeof(*global_int8_ptr);
+int sizeof_int8_var_ptr = sizeof(global_int8_ptr);
+
+extern fixed_int8_t *extern_int8_ptr;
+
+int alignof_int8 = __alignof__(extern_int8);
+int alignof_int8_var = __alignof__(*extern_int8_ptr);
+int alignof_int8_var_ptr = __alignof__(extern_int8_ptr);
+
+void f(int c) {
+  fixed_int8_t fs8;
+  svint8_t ss8;
+
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
+}
+
+// --//
+// Sizeof
+
+#define VECTOR_SIZE ((N / 8))
+#define PRED_SIZE ((N / 64))
+
+_Static_assert(sizeof(fixed_int8_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_int16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_uint8_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_float16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_float64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bfloat16_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_bool_t) == PRED_SIZE, "");
+
+// --//
+// Alignof
+
+#define VECTOR_ALIGN 16
+#define PRED_ALIGN 2
+
+_Static_assert(__alignof__(fixed_int8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_int64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_uint8_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint32_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_uint64_t) == VECTOR_ALIGN, "");
+
+_Static_assert(__alignof__(fixed_float16_t) == VECTOR_ALIGN, "");
+_Static_assert(__alignof__(fixed_float32_t) == VECTOR_ALIGN, "");

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

c-rhodes wrote:
> sdesmalen wrote:
> > aaron.ballman wrote:
> > > sdesmalen wrote:
> > > > nit: Can you add a comment saying why these are undocumented (and have 
> > > > no spellings)
> > > Also, I think these are all missing `let SemaHandler = 0;` and `let 
> > > ASTNode = 0;`
> > > 
> > > Is there a reason why we need N different type attributes instead of 
> > > having a single type attribute which encodes the N as an argument? I 
> > > think this may simplify the patch somewhat as you no longer need to 
> > > switch over N as much.
> > > Is there a reason why we need N different type attributes instead of 
> > > having a single type attribute which encodes the N as an argument?
> > AIUI this was a workaround for getting the value of N from an 
> > AttributedType, because this only has `getAttrKind` to return the attribute 
> > kind, but no way to get the corresponding argument/value. This seemed like 
> > a simple way to do that without having to create a new subclass for Type 
> > and having to support that in various places. Is the latter the approach 
> > you were thinking of? (or is there perhaps a simpler way?)
> > Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 
> > 0;
> 
> Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A, 
> CurType, CurType);` which gives an `AttributedType` in the AST, should I 
> still set `let ASTNode = 0;` in this case?
> 
> > Is there a reason why we need N different type attributes instead of having 
> > a single type attribute which encodes the N as an argument?
> 
> As Sander mentioned, it seemed like the easiest solution, interested to know 
> if there's a better approach however
I was thinking specifically of creating a new `Type` subclass and supporting it 
rather than adding 5 new attributes that only vary by a bit-width (which means 
there's likely to be a 6th someday). It's not immediately clear to me whether 
that's a really big ask for little gain or not, though.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

aaron.ballman wrote:
> c-rhodes wrote:
> > sdesmalen wrote:
> > > aaron.ballman wrote:
> > > > sdesmalen wrote:
> > > > > nit: Can you add a comment saying why these are undocumented (and 
> > > > > have no spellings)
> > > > Also, I think these are all missing `let SemaHandler = 0;` and `let 
> > > > ASTNode = 0;`
> > > > 
> > > > Is there a reason why we need N different type attributes instead of 
> > > > having a single type attribute which encodes the N as an argument? I 
> > > > think this may simplify the patch somewhat as you no longer need to 
> > > > switch over N as much.
> > > > Is there a reason why we need N different type attributes instead of 
> > > > having a single type attribute which encodes the N as an argument?
> > > AIUI this was a workaround for getting the value of N from an 
> > > AttributedType, because this only has `getAttrKind` to return the 
> > > attribute kind, but no way to get the corresponding argument/value. This 
> > > seemed like a simple way to do that without having to create a new 
> > > subclass for Type and having to support that in various places. Is the 
> > > latter the approach you were thinking of? (or is there perhaps a simpler 
> > > way?)
> > > Also, I think these are all missing let SemaHandler = 0; and let ASTNode 
> > > = 0;
> > 
> > Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A, 
> > CurType, CurType);` which gives an `AttributedType` in the AST, should I 
> > still set `let ASTNode = 0;` in this case?
> > 
> > > Is there a reason why we need N different type attributes instead of 
> > > having a single type attribute which encodes the N as an argument?
> > 
> > As Sander mentioned, it seemed like the easiest solution, interested to 
> > know if there's a better approach however
> I was thinking specifically of creating a new `Type` subclass and supporting 
> it rather than adding 5 new attributes that only vary by a bit-width (which 
> means there's likely to be a 6th someday). It's not immediately clear to me 
> whether that's a really big ask for little gain or not, though.
Ah, you're right, we may still need `ASTNode` to be kept around for that, good 
call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

sdesmalen wrote:
> aaron.ballman wrote:
> > sdesmalen wrote:
> > > nit: Can you add a comment saying why these are undocumented (and have no 
> > > spellings)
> > Also, I think these are all missing `let SemaHandler = 0;` and `let ASTNode 
> > = 0;`
> > 
> > Is there a reason why we need N different type attributes instead of having 
> > a single type attribute which encodes the N as an argument? I think this 
> > may simplify the patch somewhat as you no longer need to switch over N as 
> > much.
> > Is there a reason why we need N different type attributes instead of having 
> > a single type attribute which encodes the N as an argument?
> AIUI this was a workaround for getting the value of N from an AttributedType, 
> because this only has `getAttrKind` to return the attribute kind, but no way 
> to get the corresponding argument/value. This seemed like a simple way to do 
> that without having to create a new subclass for Type and having to support 
> that in various places. Is the latter the approach you were thinking of? (or 
> is there perhaps a simpler way?)
> Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 0;

Good to know. In SemaType I'm doing `CurType = State.getAttributedType(A, 
CurType, CurType);` which gives an `AttributedType` in the AST, should I still 
set `let ASTNode = 0;` in this case?

> Is there a reason why we need N different type attributes instead of having a 
> single type attribute which encodes the N as an argument?

As Sander mentioned, it seemed like the easiest solution, interested to know if 
there's a better approach however


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

aaron.ballman wrote:
> sdesmalen wrote:
> > nit: Can you add a comment saying why these are undocumented (and have no 
> > spellings)
> Also, I think these are all missing `let SemaHandler = 0;` and `let ASTNode = 
> 0;`
> 
> Is there a reason why we need N different type attributes instead of having a 
> single type attribute which encodes the N as an argument? I think this may 
> simplify the patch somewhat as you no longer need to switch over N as much.
> Is there a reason why we need N different type attributes instead of having a 
> single type attribute which encodes the N as an argument?
AIUI this was a workaround for getting the value of N from an AttributedType, 
because this only has `getAttrKind` to return the attribute kind, but no way to 
get the corresponding argument/value. This seemed like a simple way to do that 
without having to create a new subclass for Type and having to support that in 
various places. Is the latter the approach you were thinking of? (or is there 
perhaps a simpler way?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

sdesmalen wrote:
> nit: Can you add a comment saying why these are undocumented (and have no 
> spellings)
Also, I think these are all missing `let SemaHandler = 0;` and `let ASTNode = 
0;`

Is there a reason why we need N different type attributes instead of having a 
single type attribute which encodes the N as an argument? I think this may 
simplify the patch somewhat as you no longer need to switch over N as much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];

nit: Can you add a comment saying why these are undocumented (and have no 
spellings)



Comment at: clang/lib/AST/ASTContext.cpp:1872
 
+bool getSveVectorWidth(const Type *T, unsigned ) {
+  if (T->hasAttr(attr::ArmSveVectorBits128))

Should this function just return `unsigned` and error when it doesn't have any 
of the ArmSveVectorBits attributes?
i.e. if `isVLST()` returns true, then it is an error if it doesn't have any of 
the attributes handled below.



Comment at: clang/lib/AST/ASTContext.cpp:1897
+
+bool ASTContext::getArmSveVectorBits(const Type *T, unsigned ) const {
+  if (!T->isVLST())

nit: I find this name a bit misleading, because I would expect the 
(ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename this 
to `getBitwidthForAttributedSveType` ?



Comment at: clang/lib/Sema/SemaType.cpp:7754
 /// the ACLE, such as svint32_t and svbool_t.
-static void HandleArmSveVectorBitsTypeAttr(QualType ,
-   const ParsedAttr , Sema ) {
+static void HandleArmSveVectorBitsTypeAttr(TypeProcessingState ,
+   QualType ,

Unrelated changes?



Comment at: clang/lib/Sema/SemaType.cpp:7839
+  default:
+llvm_unreachable("unsupported vector size!");
+  case 128:

If we only support power-of-two for now, we should only have an 
`llvm_unreachable` if we prevent parsing any of the other widths (and give an 
appropriate diagnostic saying those widths are not yet supported).



Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:45
+
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;

nit: For the tests that you've added below, can you add more elaborate comments 
explaining what you're trying to test?
e.g. here I assume that sizeless globals are otherwise not allowed, but they 
are when attributed with arm_sve_vector_bits. It would be good to have that 
explained a bit.



Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:80
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types 
('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types 
('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}

Is this diagnostic produced because of any code-changes in this patch?



Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:136
+
+struct struct_int8 { fixed_int8_t x, y[5]; };
+struct struct_int16 { fixed_int16_t x, y[5]; };

nit: Is it necessary to test this for all the types?



Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:206
+// --//
+// Test call
+

nit: s/Test call/Test the scalable and fixed-width annotated types can be used 
interchangeably/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83551



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

[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes created this revision.
c-rhodes added reviewers: sdesmalen, rsandifo-arm, efriedma, cameron.mcinally, 
ctetreau.
Herald added subscribers: danielkiss, kristof.beyls, tschuett.
Herald added a reviewer: rengolin.
Herald added a reviewer: aaron.ballman.
Herald added a project: clang.

This patch implements semantics for the 'arm_sve_vector_bits' type
attribute, defined by the Arm C Language Extensions (ACLE) for SVE [1].
The purpose of this attribute is to define fixed-length (VLST) versions
of existing sizeless types (VLAT).

Implemented in this patch is the the behaviour described in section 3.7.3.2
and minimal parts of sections 3.7.3.3 and 3.7.3.4, this includes:

- Defining VLST globals, structs, unions, and local variables
- Implicit casting between VLAT <=> VLST.
- Diagnosis of ill-formed conditional expressions of the form:

  C ?  E1 : E2

  where E1 is a VLAT type and E2 is a VLST, or vice-versa. This avoids any 
ambiguity about the nature of the result type (i.e is it sized or sizeless).
- For vectors:
  - sizeof(VLST) == N/8
  - alignof(VLST) == 16
- For predicates:
  - sizeof(VLST) == N/64
  - alignof(VLST) == 2

VLSTs have the same representation as VLATs in the AST but are wrapped
with a TypeAttribute. Scalable types are currently emitted in the IR for
uses such as globals and structs which don't support these types, this
is addressed in the next patch with codegen, where VLSTs are lowered to
sized arrays for globals, structs / unions and arrays.

Not implemented in this patch is the behaviour guarded by the feature
macros:

- __ARM_FEATURE_SVE_VECTOR_OPERATORS
- __ARM_FEATURE_SVE_PREDICATE_OPERATORS

As such, the GNU __attribute__((vector_size)) extension is not available
and operators such as binary '+' are not supported for VLSTs. Support
for this is intended to be addressed by later patches.

[1] https://developer.arm.com/documentation/100987/latest


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83551

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/attr-arm-sve-vector-bits.c

Index: clang/test/Sema/attr-arm-sve-vector-bits.c
===
--- clang/test/Sema/attr-arm-sve-vector-bits.c
+++ clang/test/Sema/attr-arm-sve-vector-bits.c
@@ -41,3 +41,194 @@
 typedef float badtype3 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'float'}}
 typedef svint8x2_t badtype4 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svint8x2_t' (aka '__clang_svint8x2_t')}}
 typedef svfloat32x3_t badtype5 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'arm_sve_vector_bits' attribute applied to non-SVE type 'svfloat32x3_t' (aka '__clang_svfloat32x3_t')}}
+
+fixed_int8_t global_int8;
+fixed_bfloat16_t global_bfloat16;
+fixed_bool_t global_bool;
+
+extern fixed_int8_t extern_int8;
+extern fixed_bfloat16_t extern_bfloat16;
+extern fixed_bool_t extern_bool;
+
+static fixed_int8_t static_int8;
+static fixed_bfloat16_t static_bfloat16;
+static fixed_bool_t static_bool;
+
+fixed_int8_t* global_int8_ptr;
+extern fixed_int8_t* extern_int8_ptr;
+static fixed_int8_t* static_int8_ptr;
+__thread fixed_int8_t thread_int8;
+
+typedef fixed_int8_t int8_typedef;
+typedef fixed_int8_t *int8_ptr_typedef;
+
+int sizeof_int8 = sizeof(global_int8);
+int sizeof_int8_var = sizeof(*global_int8_ptr);
+int sizeof_int8_var_ptr = sizeof(global_int8_ptr);
+
+extern fixed_int8_t *extern_int8_ptr;
+
+int alignof_int8 = __alignof__(extern_int8);
+int alignof_int8_var = __alignof__(*extern_int8_ptr);
+int alignof_int8_var_ptr = __alignof__(extern_int8_ptr);
+
+void f(bool c) {
+  fixed_int8_t fs8;
+  svint8_t ss8;
+
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
+}
+
+// --//
+// Sizeof
+
+#define VECTOR_SIZE ((N / 8))
+#define PRED_SIZE ((N / 64))
+
+_Static_assert(sizeof(fixed_int8_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_int16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_int64_t) == VECTOR_SIZE, "");
+
+_Static_assert(sizeof(fixed_uint8_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint16_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint32_t) == VECTOR_SIZE, "");
+_Static_assert(sizeof(fixed_uint64_t) == VECTOR_SIZE,