This patch implements the basic parsing and semantic analysis of `#pragma omp declare variant`: * registers '#pragma omp declare variant' alongside other OpenMP pragmas * parses the 'variant(variant-func-id)' clause and verifies that 'variant-func-id' corresponds to a previously declared function * template-is as 'variant-func-id' is not supported * '#pragma omp declare variant` is not allowed in front of template declarations. Note that this patch will only parse a subset of the required clauses, e.g. 'variant(variant-func-id)'. Any remaining tokens before 'tok::annot_pragma_openmp_end' are ignored. As per OpenMP 5 spec, this form of '#pragma omp declare variant' is incomplete and invalid and will be rejected with an error once the implementation is complete. In the meantime, this reduced form is treated as valid.
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
include/clang/Basic/Attr.td | ||
---|---|---|
3202 ↗ | (On Diff #219142) | For the first patch we don't need the attribute, just parsing/sema analysis |
3220 ↗ | (On Diff #219142) | This is definitely wrong, especially if we need to handle templates. |
lib/Parse/ParseOpenMP.cpp | ||
778–782 |
| |
832 | /*IsReinject=*/ | |
834 | Same here | |
lib/Sema/SemaOpenMP.cpp | ||
4886 | In this patch need to implement just basic checks, no need to create attributes, etc. I would suggest to look at the checks for Target Multiversioning attribite (see D40819) |
- Removed declare variant from Attr.td
- Moved the 'vector-var-id' lookup from ParseOpenMP.cpp to SemaOpenMP.cpp
- Parsing 'vector-var-id' as an expression
- Removed the creation of the attribute in ActOnOpenMPDeclareVariantDirective
I've addressed most of the comments except for those related to templates. I'd like to clarify as what is the expected behaviour there before proceeding with implementation.
include/clang/Basic/Attr.td | ||
---|---|---|
3220 ↗ | (On Diff #219142) | What else would you use? Also, we're not handling template-ids as 'variant-func-id' just yet. |
lib/Parse/ParseOpenMP.cpp | ||
778–782 |
Re 2. What the interaction between templates and #pragma(s) should be? The C++ standard doesn't specify that, but IMHO mixing '#pragma omp declare variant' and templates requires that interaction to be clearly defined. I know that OpenMP 5 allows 'variant-func-id' to be a template-id, but I couldn't find an example of any attribute that would use C++ template parameters. Also, isn't parsing of attributes and templates _orthogonal_ in clang (i.e. happening at completely different points during parsing)? In any case, we felt that support for 'template-id' could come later. How do you feel about it? | |
lib/Sema/SemaOpenMP.cpp | ||
4886 | This makes sense, but do you reckon that that should be split into separate functions? I was inspired by the implementation of declare simd. From what I can tell, the only Sema checking for declare simd is happening in ActOnOpenMPDeclareVariantDirective. It feels that doing the same for declare variant would be good for consistency. To limit this method to Sema checks, I will remove the call to CreateImplicit. |
I left some minor notes but overall I think we should put this in and improve on it in-tree. @ABataev, would that be OK with you?
lib/Parse/ParseOpenMP.cpp | ||
---|---|---|
748 | Remove the "vector" parts in the descriptions and variable names below please, it is not "vector" specific. Nit: I'd remove the note justifying the choice for a member function. | |
788 | I actually doubt the "vec-var-id" has to be an tok::identifier but let's put that discussion on hold for later patches and get it in for the most common case, identifiers. | |
lib/Sema/SemaOpenMP.cpp | ||
4930 | I'm a little unsure about the template instantiation check (have to actually open the standard on that) but I guess being to conservative now and making progress is an acceptable way to go anyway. |
include/clang/Basic/Attr.td | ||
---|---|---|
3220 ↗ | (On Diff #219142) | But we have to handle templates from the very beginning. This is the thing that would be very hard to support later and we have to think about it in the first patch. |
lib/Parse/ParseOpenMP.cpp | ||
778–782 | No, don't try to parse function names directly. What about ::fun format? Or std::fun? Your solution does not support it. | |
788 | This is definitely wrong. |
lib/Parse/ParseOpenMP.cpp | ||
---|---|---|
778–782 | That's true - my approach doesn't support ::fun or std::fun. The idea is to fill in the gaps as we progress later. @ABataev, If you have a more generic implementation then it makes a lot of sense to go with that instead. Please, feel free to re-use this patch if that helps. |
Remove the "vector" parts in the descriptions and variable names below please, it is not "vector" specific.
Nit: I'd remove the note justifying the choice for a member function.