[PATCH] D67294: Register and parse a simplified version of '#pragma omp declare variant'

[Andrzej Warzynski via Phabricator via cfe-commits]

andwar added a comment.

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.



================
Comment at: include/clang/Basic/Attr.td:3220
+    private:
+      NamedDecl *VecVarNamedDecl;
+
----------------
ABataev wrote:
> This is definitely wrong, especially if we need to handle templates.
What else would you use? Also, we're not handling template-ids as 'variant-func-id' just yet.


================
Comment at: lib/Parse/ParseOpenMP.cpp:779-783
+  auto Identinfo = PP.getIdentifierInfo(VectorVariantId.Ident->getName());
+  LookupResult Lookup(Actions, Identinfo, VectorVariantId.Loc,
+                      Sema::LookupOrdinaryName);
+  CXXScopeSpec SS;
+  if (!Actions.LookupParsedName(Lookup, getCurScope(), &SS)) {
----------------
ABataev wrote:
> 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.
1. That's fine, let me move it there.

2. TL;DR We're not handling templates yet. 

3. 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?


================
Comment at: lib/Sema/SemaOpenMP.cpp:4886
 
+Sema::DeclGroupPtrTy Sema::ActOnOpenMPDeclareVariantDirective(
+    DeclGroupPtrTy DG, IdentifierLoc &VectorVariantId, SourceRange SR) {
----------------
ABataev wrote:
> 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)
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`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67294/new/

https://reviews.llvm.org/D67294