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

[Eli Friedman via Phabricator via cfe-commits]

efriedma 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")) {
----------------
c-rhodes wrote:
> 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.
> 
> 
I don't think it makes sense to try to parse the value of the __ARM_FEATURE_SVE_BITS out of the macro.  The macro should be defined by the compiler itself, so we should have the value stored somewhere else.


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

https://reviews.llvm.org/D83550