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

[Alexey Bataev]

ABataev added a comment.

Richard, thanks for the review. I'll try to fix printing of pragmas.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2816
@@ -2818,1 +2815,3 @@
 
+Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclarationWithPragmas(
+    AccessSpecifier &AS, ParsedAttributesWithRange &AccessAttrs,
----------------
rsmith wrote:
> Please can you split this refactoring out into a separate commit? That'll make the functional change here (handling `annot_pragma_openmp`) much easier for future code archaeologists to see.
Ok, done

================
Comment at: lib/Parse/ParseOpenMP.cpp:50-73
@@ -48,26 +49,26 @@
   for (unsigned i = 0; i < llvm::array_lengthof(F); ++i) {
-    if (!Tok.isAnnotation() && DKind == OMPD_unknown) {
+    if (!Tok.isAnnotation() && DKind == OMPD_unknown)
       TokenMatched =
-          (i == 0) &&
-          !P.getPreprocessor().getSpelling(Tok).compare("cancellation");
-    } else {
+          ((i == 0) &&
+           !P.getPreprocessor().getSpelling(Tok).compare("declare")) ||
+          ((i == 1) &&
+           !P.getPreprocessor().getSpelling(Tok).compare("cancellation"));
+    else
       TokenMatched = DKind == F[i][0] && DKind != OMPD_unknown;
-    }
 
     if (TokenMatched) {
       Tok = P.getPreprocessor().LookAhead(0);
       auto TokenIsAnnotation = Tok.isAnnotation();
       auto SDKind =
           TokenIsAnnotation
               ? OMPD_unknown
               : getOpenMPDirectiveKind(P.getPreprocessor().getSpelling(Tok));
 
       if (!TokenIsAnnotation && SDKind == OMPD_unknown) {
         TokenMatched =
-            ((i == 0) &&
+            ((i == 1) &&
              !P.getPreprocessor().getSpelling(Tok).compare("point")) ||
-            ((i == 1) && !P.getPreprocessor().getSpelling(Tok).compare("data"));
-      } else {
+            ((i == 2) && !P.getPreprocessor().getSpelling(Tok).compare("data"));
+      } else
         TokenMatched = SDKind == F[i][1] && SDKind != OMPD_unknown;
 
----------------
rsmith wrote:
> Bracing here is now inconsistent.
Fixed, thanks

================
Comment at: lib/Parse/ParseOpenMP.cpp:150
@@ +149,3 @@
+                                                       TagDecl);
+    else
+      // Here we expect to see some function declaration.
----------------
rsmith wrote:
> Add braces around this multi-line `else` (and the corresponding `if` for consistency).
Done, thanks

================
Comment at: lib/Parse/ParseOpenMP.cpp:154
@@ +153,3 @@
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none || TagType == DeclSpec::TST_unspecified) {
+          MaybeParseCXX11Attributes(Attrs);
----------------
rsmith wrote:
> Do you really need both these checks?
Removed the second one and turned it to assert()

================
Comment at: lib/Parse/ParseOpenMP.cpp:171-177
@@ +170,9 @@
+
+    // Append the current token at the end of the new token stream so that it
+    // doesn't get lost.
+    CachedPragmas.push_back(Tok);
+    // Push back tokens for pragma.
+    PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),
+                        /*DisableMacroExpansion=*/true,
+                        /*OwnsTokens=*/false);
+    // Parse pragma itself.
----------------
rsmith wrote:
> Why are you doing this delayed parsing? You still seem to have no tests that require it.
We need it for future parsing of clauses associated with the 'declare simd' construct. Some of the clauses may have references to function arguments. That's why I'm using delayed parsing: at first we need to parse a function along with arguments and only then we'll parse all the pragmas along with their clauses and references to args.
Of course currently it is not used, because this patch does not introduce parsing of clauses. It will be added later.

================
Comment at: lib/Sema/SemaOpenMP.cpp:2153
@@ +2152,3 @@
+Sema::DeclGroupPtrTy
+Sema::ActOnOpenMPDeclareSimdDirective(ArrayRef<OMPClause *> Clauses,
+                                      Decl *ADecl, SourceLocation StartLoc) {
----------------
rsmith wrote:
> You don't use `Clauses` for anything in here; is that intentional?
It will be used later when we'll add parsing of clauses for this pragma


http://reviews.llvm.org/D10599