fpetrogalli added a comment. Hi @c-rhodes, Peter asked me to take a look at this. LGTM, I only have minor stuff.
In the commit message: > Arm C Language Extensions (ACLE, version 00bet5, section 3.7.3.3) for SVE [1]. It seems that a reference to [1] is missing: https://developer.arm.com/documentation/100987/latest Also, please note that the current version is 00bet6. Nothing seem to have changed from 00bet5 to 00bet6 in terms of this patch, but I think it is worth keeping it up to date with the specs numbering until it is merged into master. Please fix the commit message before submitting. Other than that, I only have a couple of nits that I'll let you decide what to do. Thanks! Francesco ================ Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:137 + + sel = c ? gs8 : fs8; // expected-error {{cannot combine GNU and SVE vectors in expression, result is ambiguous}} + sel = c ? fs8 : gs8; // expected-error {{cannot combine GNU and SVE vectors in expression, result is ambiguous}} ---------------- Nit: I spent some time trying to verify that this was the right test. It would be grate if each test had some reference about the item and page stating the rule that the test is testing. Too bad that the items in the spec don't have a number that can easily referred to... ================ Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:141-158 + ss8 = ss8 + fs8; // expected-error {{cannot combine fixed-length and sizeless SVE vectors in expression, result is ambiguous}} + ss8 = ss8 + gs8; // expected-error {{cannot combine GNU and SVE vectors in expression, result is ambiguous}} + + fs8 = fs8 + ss8; // expected-error {{cannot combine fixed-length and sizeless SVE vectors in expression, result is ambiguous}} + fs8 = fs8 + gs8; // expected-error {{cannot combine GNU and SVE vectors in expression, result is ambiguous}} + + gs8 = gs8 + ss8; // expected-error {{cannot combine GNU and SVE vectors in expression, result is ambiguous}} ---------------- Nit: should you test more binary operators other than just `+`, like you have done for the vector initialization tests? ================ Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:300 + +#if defined(__ARM_FEATURE_SVE_BITS) && __ARM_FEATURE_SVE_BITS == 256 + ---------------- Nit: the `define(...)` seems redundant. Can this be `#if __ARM_FEATURE_SVE_BITS == 256`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88233/new/ https://reviews.llvm.org/D88233 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
