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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen 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")) {
----------------
efriedma wrote:
> 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.
> 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.
You're right, I mistook this Sema function for parsing function, my bad.

> 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;
> }```
You can probably search for the value of `__arm_sve_feature_bits` at a global scope rather than the current scope. It could also be an internal state variable in Clang. As long as we don't have to rely on the value of the macro while we're parsing the file. I'm personally not too bothered if you want to do this in a separate patch while you're still implementing the feature, or if you want to update this patch, but it needs to get fixed because parsing the macro is the wrong way around.

> 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.
When this feature is implemented, the `-msve-vector-bits` driver flag is the only interface to set the vector-length, any other way would be invalid.


================
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;
----------------
c-rhodes wrote:
> sdesmalen wrote:
> > `ArmSveFeatureBits` can be `nullptr`. Or it shouldn't use `dyn_cast`.
> I've added a check + diagnostic if it's a nullptr
Thanks.


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

https://reviews.llvm.org/D83550