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

[Mike Rice via Phabricator via llvm-commits]

mikerice marked 6 inline comments as done.
mikerice added a comment.

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.



================
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<
----------------
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.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1451
+              << getOpenMPClauseName(CKind) << 0;
+          IsError = true;
+        }
----------------
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.  


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1573-1574
+    Diag(Tok.getLocation(), diag::err_omp_unexpected_append_op);
+    SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
+              StopBeforeMatch);
+  }
----------------
aaron.ballman wrote:
> Does this need to be a while loop because OpenMP is weird about SkipUntil?
I think this one is okay. It will stop at the r_paren and consume it here.  The caller will skip the tokens until the end of the directive.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7025-7027
+      QualType RetTy = Context.VoidTy;
+      PTy = dyn_cast<FunctionProtoType>(
+          Context.getFunctionType(RetTy, None, EPI));
----------------
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. 


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

https://reviews.llvm.org/D111854