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