[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 inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:7784
+  // The __ARM_FEATURE_SVE_BITS macro must be defined when using this attribute.
+  auto &PP = S.getPreprocessor();
+  if (!PP.isMacroDefined("__ARM_FEATURE_SVE_BITS")) {
----------------
sdesmalen wrote:
> I think that the code that checks if the value of the attribute matches the value set by -msve-vector-bits should be part of Sema, not the parser. Also I'm tempted to suggest decoupling the value of the macro from the code that checks the attribute-value matches -msve-vector-bits.
> 
> If the arm_sve.h header file defines a constant value like this:
> ```#if defined(__ARM_SVE_FEATURE_BITS)
> const unsigned __arm_sve_feature_bits = __ARM_SVE_FEATURE_BITS
> #endif```
> You can check for the availability and value of this constant in the AST during semantic analysis. That way, if for whatever reason the value of the macro is redefined, the compiler can issue a diagnostic. Ideally we would insert a `__arm_sve_feature_bits` constant into the compilation unit directly when -msve-vector-bits is passed, but I don't know Clang enough to suggest where or at which point to add that.
> I think that the code that checks if the value of the attribute matches the value set by -msve-vector-bits should be part of Sema, not the parser.
This code which is checking `N==__ARM_FEATURE_SVE_BITS` is in Sema, maybe there's a more suitable place I'm not aware of but I think it makes sense to check this when looking at the type attribute.

> That way, if for whatever reason the value of the macro is redefined, the compiler can issue a diagnostic. 
I'm not convinced having a constant in the header fixes that, I suspect the user could redefine that constant as they could the macro, e.g.:
```void f() {
  const unsigned __arm_sve_feature_bits = 512;
}```

Ideally we want to diagnose inconsistent vector-lengths since we don't support it,  but for the time being maybe we can be explicit about what we do support and encourage users to use the `-msve-vector-bits` flag.




================
Comment at: clang/lib/Sema/SemaType.cpp:7818
+  unsigned VecSize = static_cast<unsigned>(SveVectorSizeInBits.getZExtValue());
+  if (ArmSveFeatureBits->getValue() != VecSize) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_bad_sve_vector_size) << VecSize;
----------------
sdesmalen wrote:
> `ArmSveFeatureBits` can be `nullptr`. Or it shouldn't use `dyn_cast`.
I've added a check + diagnostic if it's a nullptr


================
Comment at: clang/test/Sema/arm-feature-sve-bits-macro.c:1
+// RUN: %clang_cc1 -triple aarch64 -target-feature +sve -fsyntax-only -verify -D__ARM_FEATURE_SVE_BITS=512 -D__ARM_FEATURE_SVE -fallow-half-arguments-and-returns %s
+
----------------
sdesmalen wrote:
> nit: `-D__ARM_FEATURE_SVE` is no longer necessary.
Missed that one, cheers!


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

https://reviews.llvm.org/D83550