[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/include/clang/Basic/DiagnosticSemaKinds.td:2806
+def err_attribute_bad_sve_vector_size : Error<
+  "unsupported SVE vector size '%0'">;
+def err_attribute_arm_feature_sve_bits_macro_not_defined : Error<
----------------
Do you want to be more specific in the wording here by saying that it doesn't match the value set by -msve-vector-bits?


================
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")) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaType.cpp:7795
+  if (!MI)
+    llvm_unreachable("Bad macro!");
+
----------------
Should this just be an `assert` instead? `MI` should never be nullptr if `isMacrodefined(..)` returns `true`.


================
Comment at: clang/lib/Sema/SemaType.cpp:7809
+    S.Diag(Attr.getLoc(),
+           diag::err_attribute_arm_feature_sve_bits_macro_argument_type);
+    Attr.setInvalid();
----------------
__ARM_FEATURE_SVE_BITS is never supposed to be set by the user, so the parsing and diagnostic can be simplified, e.g.
```if(MI->getNumTokens() != 1 || MI->tokens().front().isNot(tok::numeric_constant)) {
  S.Diag(Attr.getLoc(), diag::err_arm_feature_sve_bits_macro_broken)
  Attr.setInvalid();
  return;
}```
but this is probably no longer relevant if you implement the suggestion above on line 7784.


================
Comment at: clang/lib/Sema/SemaType.cpp:7815
+  // The attribute vector size must equal __ARM_FEATURE_SVE_BITS.
+  ExprResult ExprRes = S.ActOnNumericConstant(ArmFeatureSveBitsTok);
+  const auto *ArmSveFeatureBits = dyn_cast<IntegerLiteral>(ExprRes.get());
----------------
nit: `ExprRes` is not very descriptive, how about `ArmFeatureSveBitsMacroExprRes` ?


================
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;
----------------
`ArmSveFeatureBits` can be `nullptr`. Or it shouldn't use `dyn_cast`.


================
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
+
----------------
nit: `-D__ARM_FEATURE_SVE` is no longer necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83550