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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
