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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
