[PATCH] D83550: [PATCH 1/4][Sema][AArch64] Add parsing support for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

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

@aaron.ballman thanks for comments! I've updated the patch



================
Comment at: clang/include/clang/Basic/Attr.td:1538
+  let Args = [IntArgument<"NumBits">];
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> No new, undocumented attributes, please.
I've added some docs


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:3
+
+#include <arm_sve.h>
+
----------------
aaron.ballman wrote:
> This should not be using a system include (unless you control the include path from the RUN line so this doesn't depend on the system running the test).
Good spot, I missed `// REQUIRES: aarch64-registered-target` which we use in the other ACLE tests.


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:4
+#include <arm_sve.h>
+
+#define N 512
----------------
aaron.ballman wrote:
> You should have a test that the attribute works at this location due to the macro being defined on the command line.
I think that's covered by `clang/test/Sema/attr-arm-sve-vector-bits.c`


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:22
+// SVE vector bits must equal __ARM_FEATURE_SVE_BITS
+#define __ARM_FEATURE_SVE_BITS 512
+typedef svint8_t badsize __attribute__((arm_sve_vector_bits(256))); // expected-error {{inconsistent SVE vector size '256', must match __ARM_FEATURE_SVE_BITS feature macro set by -msve-vector-bits}}
----------------
aaron.ballman wrote:
> I'd also appreciate test cases like:
> ```
> #define __ARM_FEATURE_SVE_BITS(x)  512
> #define __ARM_FEATURE_SVE_BITS N
> ```
> 
> #define __ARM_FEATURE_SVE_BITS N
Good point, I've added a test for this which I think covers the `isValueDependent` check.

> #define __ARM_FEATURE_SVE_BITS(x)  512
I did try this but no diagnostic is emitted, were you thinking this would cover `isTypeDependent`? To be honest I copied that from `HandleNeonVectorTypeAttr` and I'm not sure if it's necessary or what a test would look like for it. The tests for `neon_vector_type` only cover non-ICE "2.0".


================
Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:7
+
+#include <arm_sve.h>
+
----------------
aaron.ballman wrote:
> Same issue here as above.
As above, I've added `// REQUIRES: aarch64-registered-target`


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

https://reviews.llvm.org/D83550