[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/test/Sema/arm-feature-sve-bits-macro.c:24
+#define __ARM_FEATURE_SVE_BITS N
+typedef svint8_t macro_non_int_size2 __attribute__((arm_sve_vector_bits(N))); // expected-error {{'__ARM_FEATURE_SVE_BITS' must expand to an integer constant}}
+#undef __ARM_FEATURE_SVE_BITS
----------------
This is a case where I would have expected no diagnostic specifically because `N` does expand to an integer constant. Is there a reason to not support that?


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:3
+
+#include <arm_sve.h>
+
----------------
c-rhodes wrote:
> 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.
That makes me uncomfortable as this means you won't get any testing on platforms that may have odd behaviors, like Windows when in MS compatibility mode. I'm not keen on adding attributes that can only be tested under certain circumstances as we want to ensure behavior on all platforms.

We typically handle this by requiring the test to replicate the system header in an Inputs folder that then gets set on the RUN line so that the test can be reproduced on any platform. Was that approach considered and rejected for some reason?


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:4
+#include <arm_sve.h>
+
+#define N 512
----------------
c-rhodes wrote:
> 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`
So it is!


================
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}}
----------------
c-rhodes wrote:
> 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".
> Good point, I've added a test for this which I think covers the isValueDependent check.
FWIW, I don't think this covers the value dependent case -- that's more to do with nontype template parameters (I don't believe macros can be type or value dependent; but constant expressions can be).

> I did try this but no diagnostic is emitted, were you thinking this would cover isTypeDependent?
Nope, I don't think type dependence factors in (I consider that dependency code to be sanity-checking more than anything; it can probably be assertions if we prefer).

I was under the impression that you only want to support object-like macros and not function-like macros, even if the function-like macro results in an integer constant. If that matters, you can use `MacroInfo::isObjectLike()` or `MacroInfo::isFunctionLike()` to test for it.


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

https://reviews.llvm.org/D83550