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