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