[PATCH] [OPENMP] First changes for Parsing and Sema for 'omp simd' directive support
Richard Smith
richard at metafoo.co.uk
Tue Feb 25 16:31:31 PST 2014
================
Comment at: include/clang/AST/DataRecursiveASTVisitor.h:268-269
@@ -267,1 +267,4 @@
+ /// \brief Traverses OMPExecutableDirective class.
+ bool TraverseOMPExecutableDirective(OMPExecutableDirective *S);
+
----------------
This shouldn't be at this point in the class, it should be below with `TraverseOMPClause`.
================
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;
})
----------------
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.
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:284-285
@@ -283,1 +283,4 @@
+ /// \brief Traverses OMPExecutableDirective class.
+ bool TraverseOMPExecutableDirective(OMPExecutableDirective *S);
+
----------------
Same comments here as in `DataRecursiveASTVisitor`.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6812-6821
@@ -6811,10 +6811,12 @@
"threadprivate variable with incomplete type %0">;
def err_omp_no_dsa_for_variable : Error <
"variable %0 must have explicitly specified data sharing attributes">;
def err_omp_wrong_dsa : Error<
"%0 variable cannot be %1">;
def note_omp_explicit_dsa : Note <
"defined as %0">;
def note_omp_predetermined_dsa : Note <
"predetermined as %0">;
+def err_omp_not_for : Error <
+ "only for-loops are allowed for '#pragma omp %0'">;
} // end of OpenMP category
----------------
Don't put a space between `Error` and `<`.
================
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
----------------
In all other diagnostics, we use "for loop" not "for-loop". Also, how about
"statement after '#pragma omp %0' must be a for loop"
?
================
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);
+}
----------------
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.
================
Comment at: lib/AST/StmtPrinter.cpp:105
@@ -104,2 +104,3 @@
#include "clang/AST/StmtNodes.inc"
+ void VisitOMPExecutableDirective(OMPExecutableDirective *Node);
};
----------------
This is a weird place to add this; maybe rename it `PrintOMPExecutableDirective` and put it with the other `Print*` helpers above?
================
Comment at: lib/Sema/TreeTransform.h:622-623
@@ -621,2 +621,4 @@
+ StmtResult TransformOMPExecutableDirective(OMPExecutableDirective *S);
+
#define OPENMP_CLAUSE(Name, Class) \
----------------
Move this up to before the FIXME to keep the `.inc` / `.def` file uses together.
================
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);
+ }
+
----------------
These seem to be unused, since the `Transform*` functions call `RebuildOMPExecutableDirective` instead?
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1757
@@ +1756,3 @@
+ VisitStmt(D);
+ ++Idx;
+ VisitOMPExecutableDirective(D);
----------------
Please add a comment here indicating that the `NumClauses` field was read in `ReadStmtFromStream`.
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1763
@@ -1758,1 +1762,3 @@
+ VisitStmt(D);
+ Idx += 2;
VisitOMPExecutableDirective(D);
----------------
Likewise.
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:2254-2255
@@ +2253,4 @@
+ Empty);
+ }
+ break;
+
----------------
`break` goes inside the braces, and the close brace is dedented one step. See `case EXPR_MEMBER:` above for an example.
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:2249-2253
@@ +2248,7 @@
+ unsigned CollapsedNum = Record[ASTStmtReader::NumStmtFields + 1];
+ S = OMPSimdDirective::CreateEmpty(
+ Context,
+ NumClauses,
+ CollapsedNum,
+ Empty);
+ }
----------------
We usually don't write function arguments this sparsely. It's more normal to see this as:
S = OMPSimdDirective::CreateEmpty(Context, NumClauses,
CollapsedNum, Empty);
================
Comment at: lib/Serialization/ASTWriterStmt.cpp:1730-1731
@@ -1731,3 +1729,4 @@
}
- Writer.AddStmt(E->getAssociatedStmt());
+ if (E->getAssociatedStmt())
+ Writer.AddStmt(E->getAssociatedStmt());
}
----------------
This doesn't look right: the reader reads this field unconditionally, so it needs to be present unconditionally.
http://llvm-reviews.chandlerc.com/D2815
More information about the cfe-commits
mailing list