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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1538
+  let Args = [IntArgument<"NumBits">];
+  let Documentation = [Undocumented];
+}
----------------
No new, undocumented attributes, please.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2806
+def err_attribute_bad_sve_vector_size : Error<
+  "inconsistent SVE vector size '%0', must match __ARM_FEATURE_SVE_BITS feature macro set by -msve-vector-bits">;
+def err_attribute_arm_feature_sve_bits_macro_not_defined : Error<
----------------
Can you wrap this for 80-col limits and put some single quotes around the macro and option names?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2808
+def err_attribute_arm_feature_sve_bits_macro_not_defined : Error<
+  "__ARM_FEATURE_SVE_BITS is not defined">;
+def err_arm_feature_sve_bits_macro_broken : Error<
----------------
Single quotes around the macro name


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2810
+def err_arm_feature_sve_bits_macro_broken : Error<
+  "__ARM_FEATURE_SVE_BITS requires a single integer constant">;
 def err_attribute_requires_positive_integer : Error<
----------------
How about: `'__ARM_FEATURE_SVE_BITS' must expand to an integer constant`


================
Comment at: clang/lib/Sema/SemaType.cpp:7771
+
+  // The vector size must be an integer constant expression.
+  Expr *VecSizeExpr = static_cast<Expr *>(Attr.getArgAsExpr(0));
----------------
This code exists elsewhere in the file as well (see `HandleNeonVectorTypeAttr()`), can it be factored out into a function and called from both places?


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:3
+
+#include <arm_sve.h>
+
----------------
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).


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:4
+#include <arm_sve.h>
+
+#define N 512
----------------
You should have a test that the attribute works at this location due to the macro being defined on the command line.


================
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}}
----------------
I'd also appreciate test cases like:
```
#define __ARM_FEATURE_SVE_BITS(x)  512
#define __ARM_FEATURE_SVE_BITS N
```



================
Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:7
+
+#include <arm_sve.h>
+
----------------
Same issue here as above.


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

https://reviews.llvm.org/D83550