[PATCH] D111854: [OPENMP51]Initial parsing/sema for append_args clause for 'declare variant'

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D111854#3073851 <https://reviews.llvm.org/D111854#3073851>, @mikerice wrote:

> Thanks for the review! As far as serialization goes, yes we need it, and the OpenMP tests test this is the ast print and codegen tests.  This patch doesn't need anything special since it is just adding another field to the attribute which is serialized correctly with tablegen code afaik.

Hmmm, I took a look through the serialization code that includes OMP.inc and I don't see anything to suggest that this addition causes a change to the AST version. So the situation I'm worried about is where new Clang generates a PCH file with this feature and then hands it to an older Clang without this feature. If you think we have sufficient test coverage for this sort of thing, then okay. But if you think there's not sufficient test coverage, this might be worth a follow-up patch to improve test coverage in general.

The rest of my comments are mostly nits because I'm not really an expert in OpenMP. :-)



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1360
+def err_omp_unexpected_append_op : Error<
+  "unexpected append-op in 'append_args' clause, expected 'interop'">;
 def err_omp_declare_variant_wrong_clause : Error<
----------------
mikerice wrote:
> aaron.ballman wrote:
> > I'm hoping this diagnostic makes more sense in situ because I'm not certain what it's telling me.  `append-op` is not a term of art we've used elsewhere, is there a better term we could use or will this be familiar to OpenMP users?
> That is from the syntax in the spec: https://www.openmp.org/spec-html/5.1/openmpsu29.html#x46-450002.3.5. Open to suggestions but I don't know what would be better.
We usually try to only use grammar terms when they make sensible English prose by removing the hyphens, so I'm thinking of something along the lines of:

`unexpected operation specified in 'append_args' clause, expected 'interop'`


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10755
+  "function with '#pragma omp declare variant' must have a prototype when "
+  "append_args is used">;
+def err_omp_interop_type_not_found : Error<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10757
+def err_omp_interop_type_not_found : Error<
+  "'omp_interop_t' must be defined when append_args clause is used; include <omp.h>">;
 def err_omp_declare_variant_incompat_types : Error<
----------------



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1451
+              << getOpenMPClauseName(CKind) << 0;
+          IsError = true;
+        }
----------------
mikerice wrote:
> aaron.ballman wrote:
> > Do we need to do any error recovery here for the parser to get back to a known-perhaps-good state?
> You mean get back to a good state inside the directive? It seems we mostly just skip out of the OpenMP directives when there is an error like this does.  
Oh yeah, I see, this skips later, sorry for the noise.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7025-7027
+      QualType RetTy = Context.VoidTy;
+      PTy = dyn_cast<FunctionProtoType>(
+          Context.getFunctionType(RetTy, None, EPI));
----------------
mikerice wrote:
> aaron.ballman wrote:
> > Why is this valid? (Functions without prototypes do specify their return type information btw, so it's very weird that we convert this to return void when ginning up a prototype).
> You're right the return type is wrong here.  Thinking about this more though, I don't think we really need to handle the unprototyped case.  When append_args are used the variant must be prototyped and contain the extra omp_interop_t parameters.  That means we'd be matching a prototyped function with an unprototyped function where we treat it as (void).  That would handle only cases where the variant had only interop parameters... seems not useful.  I'll instead give an error for the unprototyped case. 
+1 -- unprototyped functions have been deprecated for longer than some WG14 committee members have been alive; I think it's a feature to not support them in new offerings.


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

https://reviews.llvm.org/D111854