[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
From:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-07-22 20:47:57
Message-ID: 852fb1844e2e5d8386196b481c2804cc () localhost ! localdomain
[Download RAW message or body]

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




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]