[PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.

[Richard Smith]

rsmith added a comment.

You have a lot of logic here to handle delayed parsing, but none of your testcases need any delayed parsing.

Are you supporting delayed parsing just to deal with the argument-list in the various clauses? (Your current delayed parsing support doesn't work for that, because the pragma is not parsed within the scope of the function prototype.) If so, have you considered producing a list of identifiers from the parser, and looking them up when the attribute is applied to the declaration in Sema? That would seem to remove the need for all of the delayed parsing.


================
Comment at: include/clang/AST/ASTMutationListener.h:118-122
@@ -117,1 +117,7 @@
 
+  /// \brief A declaration is marked as OpenMP declare simd which was not
+  /// previously marked as declare simd.
+  ///
+  /// \param D The declaration marked OpenMP declare simd.
+  virtual void DeclarationMarkedOpenMPDeclareSimd(const Decl *D) {}
+
----------------
Why do you need this? Isn't the attribute always applied to a "fresh" declaration (one that was recently created, rather than one that was imported from an external source)?

================
Comment at: include/clang/Basic/Attr.td:2059
@@ +2058,3 @@
+def OMPDeclareSimdDecl : InheritableAttr {
+  // This attribute has no spellings as it is only ever created implicitly.
+  let Spellings = [Pragma<"omp", "declare simd">];
----------------
Comment is out of date.

================
Comment at: include/clang/Basic/Attr.td:2070-2071
@@ +2069,4 @@
+  }
+  void setNumberOfDirectives(unsigned N) { numberOfDirectives = N; }
+  void setComplete() { complete = true; }
+  }];
----------------
Why do you need setters?

================
Comment at: include/clang/Parse/Parser.h:1032
@@ +1031,3 @@
+
+    /// \brief The set of tokens that make up an exception-specification that
+    /// has not yet been parsed.
----------------
exception-specification?

================
Comment at: lib/AST/DeclPrinter.cpp:412-427
@@ -409,3 +411,18 @@
 
+void DeclPrinter::print(OMPDeclareSimdDeclAttr *A) {
+  for (unsigned i = 0; i < A->getNumberOfDirectives(); ++i) {
+    A->printPrettyPragma(Out, Policy);
+    Indent();
+  }
+}
+
 void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
+  if (auto *Attr = D->getAttr<OMPDeclareSimdDeclAttr>()) {
+    if (D->getTemplatedKind() !=
+            FunctionDecl::TK_FunctionTemplateSpecialization &&
+        D->getTemplatedKind() != FunctionDecl::TK_FunctionTemplate &&
+        D->getTemplatedKind() !=
+            FunctionDecl::TK_DependentFunctionTemplateSpecialization)
+      print(Attr);
+  }
   CXXConstructorDecl *CDecl = dyn_cast<CXXConstructorDecl>(D);
----------------
This level of special-casing is not OK, please find a more general way of dealing with this.

================
Comment at: lib/Parse/ParseOpenMP.cpp:87
@@ +86,3 @@
+Parser::DeclGroupPtrTy
+Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(bool IsInTagDecl,
+                                                   unsigned Level) {
----------------
`IsInTagDecl` is the wrong name for this; enums are tag decls, but they shouldn't get this treatment.

================
Comment at: lib/Parse/ParseOpenMP.cpp:143-144
@@ +142,4 @@
+    DeclGroupPtrTy Ptr;
+    if (Tok.is(tok::annot_pragma_openmp)) {
+      Ptr = ParseOpenMPDeclarativeDirectiveWithExtDecl(IsInTagDecl, Level + 1);
+    } else {
----------------
So, you can combine "#pragma omp declare simd" with other pragmas, but only if the simd pragmas come first?

================
Comment at: lib/Parse/ParseOpenMP.cpp:244
@@ +243,3 @@
+///
+void Parser::LateParseOpenMPDeclarativeDirectiveWithTemplateFunction(
+    OpenMPDirectiveKind DKind, SourceLocation Loc) {
----------------
What does this have to do with "TemplateFunction"s?

================
Comment at: lib/Parse/ParseOpenMP.cpp:268
@@ +267,3 @@
+
+    LexTemplateFunctionForLateParsing(Decl->Tokens);
+  }
----------------
This doesn't make sense; that function is for MS-compatibility delayed template parsing, which seems entirely unrelated to this OpenMP pragma.

================
Comment at: test/OpenMP/declare_simd_ast_print.cpp:34
@@ +33,3 @@
+
+// Instatiate with <C=int> explicitly.
+// Pragmas need to be same, otherwise standard says that's undefined behavior.
----------------
This is an explicit specialization, not an instantiation.


http://reviews.llvm.org/D10599