[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 &Width) {
+  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 &Width) 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 &CurType,
-                                           const ParsedAttr &Attr, Sema &S) {
+static void HandleArmSveVectorBitsTypeAttr(TypeProcessingState &State,
+                                           QualType &CurType,
----------------
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