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:
> 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.
> 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.
I intend to set the macro in the patch adding `-msve-vector-bits`, maybe that
flag could set some internal state variable in clang as Sander suggests. I'll
look into it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83550/new/
https://reviews.llvm.org/D83550
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
