This is an archive of the discontinued LLVM Phabricator instance.

Register and parse a simplified version of '#pragma omp declare variant'
AbandonedPublic

Authored by andwar on Sep 6 2019, 11:04 AM.

Details

Reviewers
ABataev
jdoerfert
Summary
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.

Diff Detail

Repository
rC Clang

Event Timeline

andwar created this revision.Sep 6 2019, 11:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
ABataev added inline comments.Sep 6 2019, 11:18 AM
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
  1. All this lookup must be performed in Sema.
  2. How do you want to handle templates?
  3. Why just do not parse it as an expression? And rely on existing parsing/sema analysis for expressions.
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)

andwar updated this revision to Diff 219673.Sep 11 2019, 2:14 AM
andwar marked 6 inline comments as done.
  • 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
  1. That's fine, let me move it there.
  1. TL;DR We're not handling templates yet.
  1. How about switching to ParseUnqualifiedId instead? That will still require going through LookupParsedName though.

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.

ABataev added inline comments.Sep 11 2019, 3:28 AM
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.
Just parse function identifier as an expression.
Your solution also will break serialization/deserialization since attributes dies not support serilization of the custom fields.

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.
I have basic implementation that handles all these cases. Let me commit it, because we need it to support other context selectors. It is very similar to your solution but handles all the problems. We just don't have time to wait, need to support other context selectors.

788

This is definitely wrong.

andwar marked 2 inline comments as done.Sep 11 2019, 7:49 AM
andwar added inline comments.
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.

andwar abandoned this revision.Nov 21 2019, 1:03 AM