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

[Alexey Bataev via cfe-commits]

ABataev marked 7 inline comments as done.
ABataev added a comment.

Richard, thanks for the review!


================
Comment at: include/clang/Basic/Attr.td:2074-2078
@@ +2073,7 @@
+  let Documentation = [OMPDeclareSimdDocs];
+  let AdditionalMembers = [{
+  void printPrettyPragma(raw_ostream &OS, const PrintingPolicy &Policy) const {
+    OS << "\n";
+  }
+  }];
+}
----------------
rsmith wrote:
> What's this for?
This is required for proper AST printing of pragma. Without this additional member (this one is required for pragma attributes by attribute generation system) all pragmas will be printed in one line.

================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:996-999
@@ -995,2 +995,6 @@
   "expected identifier specifying the name of the 'omp critical' directive">;
+def err_omp_single_decl_in_declare_simd : Error<
+  "single declaration is expected with 'declare simd' directive">;
+def err_omp_decl_in_declare_simd : Error<
+  "function declaration is expected with 'declare simd' directive">;
 
----------------
rsmith wrote:
> with -> after, in both diagnstics.
Done, thanks.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7661
@@ -7660,1 +7660,3 @@
+def err_omp_function_expected : Error<
+  "'#pragma omp declare simd' can be applied to functions only">;
 def err_omp_wrong_cancel_region : Error<
----------------
rsmith wrote:
> can be applied to functions only -> can only be applied to functions
Done

================
Comment at: lib/AST/DeclPrinter.cpp:214
@@ +213,3 @@
+      default:
+        if (!PrintPragmas)
+          A->printPretty(Out, Policy);
----------------
rsmith wrote:
> It looks like we would no longer print attributes with pragma spelling for non-function declarations after this change (but I don't think we have any such pragmas at the moment). I don't want to ask you to write a bunch more code here that you can't test, but can you add a FIXME somewhere in here to add calls to print pragmas in more places?
Ok, will do.

================
Comment at: lib/Parse/ParseOpenMP.cpp:133
@@ +132,3 @@
+          << getOpenMPDirectiveName(OMPD_declare_simd);
+      SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
+    }
----------------
rsmith wrote:
> You should bail out if you don't actually find your desired token. If you carry on without checking, your `ConsumeToken` call might consume the `tok::eof` token or some similar bad thing.
Done, thanks.

================
Comment at: lib/Parse/ParseOpenMP.cpp:144-145
@@ +143,4 @@
+      // Here we expect to see some function declaration.
+      while (Tok.isNot(tok::r_brace) && !isEofOrEom() &&
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none) {
----------------
rsmith wrote:
> I find this loop slightly surprising: the cases where Parse*Declaration return a null `DeclGroupPtrTy` are when they parsed some declaration-like entity (such as a declaration-like pragma) but didn't create some declaration to represent it.
> 
> Do you really want to allow such things between your pragma and its associated function? As an extreme case, you'd accept things like this:
> 
>   #pragma omp declare simd
>   #pragma pack (...)
>   ;
>   public:
>   __if_exists(foo) {
>     int n;
>   }
>   int the_simd_function();
We need to be able to combine 'declare simd' pragma with other pragmas, for example:

  #pragma omp declare simd
  #pragma GCC visibility push(default)
  #pragma options align=packed
  #pragma omp declare simd
  #pragma GCC visibility pop
  int tadd(int b) { return x[b] + b; }

We should be able to accept such code. But I will reduce the number of allowed constructs.

================
Comment at: lib/Parse/ParseOpenMP.cpp:145
@@ +144,3 @@
+      while (Tok.isNot(tok::r_brace) && !isEofOrEom() &&
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none) {
----------------
rsmith wrote:
> Do you really need to check both `!Ptr` and `Ptr.get().isNull()` here? Generally, the parser shouldn't really be calling `get()` on an `OpaquePtr` -- these pointers are supposed to be opaque to the parser.
Ok, removed Ptr.get().isNull()

================
Comment at: lib/Parse/ParseOpenMP.cpp:162-168
@@ +161,9 @@
+    }
+    if (!Ptr.get().isSingleDecl())
+      Diag(Loc, diag::err_omp_single_decl_in_declare_simd);
+
+    if (Ptr.get().isSingleDecl())
+      return Actions.ActOnOpenMPDeclareSimdDirective(Ptr.get().getSingleDecl(),
+                                                     Loc);
+    return Ptr;
+  }
----------------
rsmith wrote:
> I would suggest that you instead call `ActOnOpenMPDeclareSimdDirective` unconditionally, pass in `Ptr` rather than its contained declaration, and check for a single declaration in `Sema` -- this seems much more of a semantic check than a syntactic one.
Ok, done


http://reviews.llvm.org/D10599