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