[PATCH] [OPENMP] First changes for Parsing and Sema for 'omp simd' directive support

[Alexander Musman]

================
Comment at: include/clang/AST/DataRecursiveASTVisitor.h:268-269
@@ -267,1 +267,4 @@
 
+  /// \brief Traverses OMPExecutableDirective class.
+  bool TraverseOMPExecutableDirective(OMPExecutableDirective *S);
+
----------------
Richard Smith wrote:
> This shouldn't be at this point in the class, it should be below with `TraverseOMPClause`.
Fixed.

================
Comment at: include/clang/AST/DataRecursiveASTVisitor.h:2337-2342
@@ -2333,8 +2336,8 @@
 // OpenMP directives.
-DEF_TRAVERSE_STMT(OMPParallelDirective, {
+DEF_TRAVERSE_STMT(OMPExecutableDirective, {
   ArrayRef<OMPClause *> Clauses = S->clauses();
   for (ArrayRef<OMPClause *>::iterator I = Clauses.begin(), E = Clauses.end();
        I != E; ++I)
     if (!TraverseOMPClause(*I)) return false;
 })
 
----------------
Richard Smith wrote:
> This is wrong. You don't want to use `DEF_TRAVERSE_STMT` here, because the `WalkUpFrom*` call will have already been done by the concrete type.
Fixed.

================
Comment at: lib/AST/Stmt.cpp:1243-1248
@@ +1242,8 @@
+                                                EmptyShell) {
+  void *Mem = C.Allocate(llvm::RoundUpToAlignment(
+                           sizeof(OMPSimdDirective),
+                           llvm::alignOf<OMPClause *>()) +
+                         sizeof(OMPClause *) * NumClauses + sizeof(Stmt *),
+                         llvm::alignOf<OMPSimdDirective>());
+  return new (Mem) OMPSimdDirective(CollapsedNum, NumClauses);
+}
----------------
Richard Smith wrote:
> There's no matching rounding up for alignment in `OMPExecutableDirective`'s constructor, so the padding for alignment here is really weird. You're also being inconsistent in when you pad out to alignment and when you don't, and not including the alignment in the second argument to `C.Allocate`.
> 
> I'd suggest just dropping the no-op `RoundUpToAlignment` call here, since it just provides a false sense of security and doesn't actually fix any alignment problems. If you're worried about this, maybe add an assert to `OMPExecutableDirective`'s constructor that the pointer is suitably-aligned.
Fixed.
Alexey committed alignment fixes for OMPExecutableDirective (and other directives) recently (http://llvm-reviews.chandlerc.com/D2713), and now OMPExecutableDirective's constructor does rounding up. I changed these lines to be consistent with OMPParallelDirective here (at least to do the same no-op at all places).

================
Comment at: lib/AST/StmtPrinter.cpp:105
@@ -104,2 +104,3 @@
 #include "clang/AST/StmtNodes.inc"
+    void VisitOMPExecutableDirective(OMPExecutableDirective *Node);
   };
----------------
Richard Smith wrote:
> This is a weird place to add this; maybe rename it `PrintOMPExecutableDirective` and put it with the other `Print*` helpers above?
I agree, fixed.

================
Comment at: lib/Serialization/ASTWriterStmt.cpp:1730-1731
@@ -1731,3 +1729,4 @@
   }
-  Writer.AddStmt(E->getAssociatedStmt());
+  if (E->getAssociatedStmt())
+    Writer.AddStmt(E->getAssociatedStmt());
 }
----------------
Richard Smith wrote:
> This doesn't look right: the reader reads this field unconditionally, so it needs to be present unconditionally.
Yes, both parallel and simd directives should have associated stmt. Fixed.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6821
@@ -6820,1 +6820,3 @@
+def err_omp_not_for : Error <
+  "only for-loops are allowed for '#pragma omp %0'">;
 } // end of OpenMP category
----------------
Richard Smith wrote:
> In all other diagnostics, we use "for loop" not "for-loop". Also, how about
> 
>   "statement after '#pragma omp %0' must be a for loop"
> 
> ?
I'm fine with that.

================
Comment at: lib/Sema/TreeTransform.h:1304-1326
@@ -1289,12 +1303,25 @@
+
   /// \brief Build a new OpenMP parallel directive.
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
   StmtResult RebuildOMPParallelDirective(ArrayRef<OMPClause *> Clauses,
                                          Stmt *AStmt,
                                          SourceLocation StartLoc,
                                          SourceLocation EndLoc) {
     return getSema().ActOnOpenMPParallelDirective(Clauses, AStmt,
                                                   StartLoc, EndLoc);
   }
 
+  /// \brief Build a new OpenMP simd directive.
+  ///
+  /// By default, performs semantic analysis to build the new statement.
+  /// Subclasses may override this routine to provide different behavior.
+  StmtResult RebuildOMPSimdDirective(ArrayRef<OMPClause *> Clauses,
+                                     Stmt *AStmt,
+                                     SourceLocation StartLoc,
+                                     SourceLocation EndLoc) {
+    return getSema().ActOnOpenMPSimdDirective(Clauses, AStmt,
+                                              StartLoc, EndLoc);
+  }
+
----------------
Richard Smith wrote:
> These seem to be unused, since the `Transform*` functions call `RebuildOMPExecutableDirective` instead?
I've removed these guys.

================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1757
@@ +1756,3 @@
+  VisitStmt(D);
+  ++Idx;
+  VisitOMPExecutableDirective(D);
----------------
Richard Smith wrote:
> Please add a comment here indicating that the `NumClauses` field was read in `ReadStmtFromStream`.
Done.


http://llvm-reviews.chandlerc.com/D2815