================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
