[PATCH] OpenMP parallel directive
Alexey Bataev
a.bataev at hotmail.com
Mon Apr 22 06:26:22 PDT 2013
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2272
@@ +2271,3 @@
+template <class T>
+class RecursiveOMPClauseVisitor :
+ public OMPClauseVisitor<RecursiveOMPClauseVisitor<T>, bool> {
----------------
Doug Gregor wrote:
> Why not just fold this functionality into the RecursiveASTVisitor?
Ok
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2280
@@ +2279,3 @@
+ bool Visit##Class(Class *S) { \
+ for (Stmt::child_range Range = S->children(); Range; ++Range) { \
+ if (!Visitor->TraverseStmt(*Range)) return false; \
----------------
Doug Gregor wrote:
> I wonder... rather than propagating the children() anti-pattern from Stmt/Expr, should we just implement the various Visit* methods directly in the RecursiveOMPClauseVisitor?
Ok, I'll remove children
================
Comment at: include/clang/AST/StmtOpenMP.h:32-35
@@ +31,6 @@
+class OMPClause {
+ /// \brief Starting location of the clause.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the clause.
+ SourceLocation EndLoc;
+ /// \brief Kind of the clause.
----------------
Dmitri Gribenko wrote:
> Please clarify what these locations are. We probably need locations for: clause keyword, open paren, close paren.
StartLoc - for clause keyword
EndLoc - last symbol for the clause (close paren or clause keyword, if no parameters)
I will add LParenLoc for open paren
================
Comment at: include/clang/AST/StmtOpenMP.h:58-60
@@ +57,5 @@
+
+ static bool classof(const OMPClause *T) {
+ return true;
+ }
+
----------------
Dmitri Gribenko wrote:
> I could be wrong, but IIRC this is not actually needed. dyn_cast magic should figure this out, I think.
Tried to remove these methods. Required for cast.
================
Comment at: include/clang/AST/StmtOpenMP.h:127
@@ +126,3 @@
+ ///
+ void setCondition(Expr *E) { Condition = cast<Stmt>(E); }
+public:
----------------
Dmitri Gribenko wrote:
> `cast` is not needed here.
Removed
================
Comment at: include/clang/AST/StmtOpenMP.h:136
@@ +135,3 @@
+ OMPIfClause(Expr *E, SourceLocation StartLoc, SourceLocation EndLoc)
+ : OMPClause(OMPC_if, StartLoc, EndLoc), Condition(cast<Stmt>(E)) { }
+
----------------
Dmitri Gribenko wrote:
> `cast<Stmt>` is not needed here. You could effectively replace this with `assert(E);`
Removed
================
Comment at: include/clang/AST/StmtOpenMP.h:144
@@ +143,3 @@
+ /// \brief Return condition.
+ Expr *getCondition() { return dyn_cast_or_null<Expr>(Condition); }
+
----------------
Dmitri Gribenko wrote:
> Doug Gregor wrote:
> > Wei Pan wrote:
> > > The constructors assume thatn E is not null. Either change the constructor or update the get method.
> > More directly, why are we still storing these as Stmt*? They can be Expr*, since we no longer have to have Stmt* to deal with child_iterator baggage.
> Should this member function be const-qualified?
cast is removed.
Expression is stored as Expr *.
Added const-qualified version of method.
================
Comment at: include/clang/AST/StmtOpenMP.h:190
@@ +189,3 @@
+ /// \brief Return number of threads.
+ Expr *getNumThreads() { return dyn_cast_or_null<Expr>(NumThreads); }
+
----------------
Dmitri Gribenko wrote:
> Doug Gregor wrote:
> > Wei Pan wrote:
> > > The same as above.
> > Me too :)
> Also, const-qualified?
Also the answer is still the same. :)
================
Comment at: include/clang/AST/StmtOpenMP.h:213-214
@@ +212,4 @@
+ OpenMPDefaultClauseKind Kind;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation KindLoc;
+
----------------
Dmitri Gribenko wrote:
> Maybe `KindKwLoc`?
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:230
@@ +229,3 @@
+ ///
+ /// \brief A Argument of the clause ('none' or 'shared').
+ /// \brief ALoc Starting location of the argument.
----------------
Wei Pan wrote:
> \param and thhe following \brief's
Agree
================
Comment at: include/clang/AST/StmtOpenMP.h:303-304
@@ +302,4 @@
+ ///
+ static OMPPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:349
@@ +348,3 @@
+ /// \param C AST context.
+ /// \brief StartLoc Starting location of the clause.
+ /// \brief EndLoc Ending location of the clause.
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:353
@@ +352,3 @@
+ ///
+ static OMPFirstPrivateClause *Create(ASTContext &C,
+ SourceLocation StartLoc,
----------------
Wei Pan wrote:
> Are they all DeclRefExpr? I assume the following code is vaild (gcc 4.6 compiles), you will generate a MemberExpr not a DeclRefExpr.
>
> void bar(int) { }
> struct C {
> int a;
> void foo() {
> #pragma omp firstprivate(a)
> for (int i = 0; i < 10; i++) {
> bar(a);
> }
> }
> };
>
According to OpenMP standard, 2.11.3.4 firstprivate clause, Restrictions:
A variable that is part of another variable (as an array or structure element) cannot appear in a firstprivate clause.
In this example 'a' is a structure element, which is not allowed by the standard. So, the compiler should generate error message.
================
Comment at: include/clang/AST/StmtOpenMP.h:362-363
@@ +361,4 @@
+ ///
+ static OMPFirstPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:406
@@ +405,3 @@
+ /// \param C AST context.
+ /// \brief StartLoc Starting location of the clause.
+ /// \brief EndLoc Ending location of the clause.
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:419-420
@@ +418,4 @@
+ ///
+ static OMPSharedClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:503
@@ +502,3 @@
+ OpenMPReductionClauseOperator Operator;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation OperatorLoc;
----------------
Dmitri Gribenko wrote:
> Wei Pan wrote:
> > location of the reduction operator kind.
> Actually, it is just 'location of the reduction operator'.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:563-567
@@ +562,7 @@
+
+ /// \brief Fetches operator for the clause.
+ OpenMPReductionClauseOperator getOperator() const { return Operator; }
+
+ /// \brief Fetches location of clause operator.
+ SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
----------------
Dmitri Gribenko wrote:
> Bikeshedding: s/Fetches/Returns/ everywhere?
>
> These are more or less synonyms, but 'returns' is typically used where the value being returned is not being computed or fetched from somewhere else.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:581
@@ +580,3 @@
+template <typename T> struct make_const_ptr_clause { typedef const T *type; };
+/// \brief This class implements a simple visitor for OMPClause
+/// subclasses.
----------------
Wei Pan wrote:
> Merge into the previous line.
Will be removed.
================
Comment at: include/clang/AST/StmtOpenMP.h:609
@@ +608,3 @@
+
+template<class ImplClass, typename RetTy=void>
+class OMPClauseVisitor :
----------------
Wei Pan wrote:
> A few extra whitespaces RetTy = void
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:627-630
@@ +626,6 @@
+ OpenMPDirectiveKind Kind;
+ /// \brief Starting location of the directive kind.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the directive.
+ SourceLocation EndLoc;
+ /// \brief The statement associated with the directive.
----------------
Dmitri Gribenko wrote:
> Please clarify where these point to (omp keyword? directive keyword -- like parallel? #pragma?)
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:633-636
@@ +632,6 @@
+ Stmt *AssociatedStmt;
+ /// \brief Number of clauses.
+ unsigned NumClauses;
+ /// \brief Pointer to the list of clauses.
+ OMPClause ** const Clauses;
+protected:
----------------
Dmitri Gribenko wrote:
> ArrayRef?
Yes, I'll fix it
================
Comment at: include/clang/AST/StmtOpenMP.h:645
@@ +644,3 @@
+ /// \param N Number of clauses.
+ /// \param ClausesAndStmt A pointer to the buffer for clauses.
+ ///
----------------
Dmitri Gribenko wrote:
> There's no 'ClausesAndStmt' parameter in the function declaration.
>
> Also, ArrayRef?
Yes, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:665
@@ +664,3 @@
+ ///
+ /// \brief Clauses The list of clauses for the directive.
+ ///
----------------
Dmitri Gribenko wrote:
> \param CL
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:671
@@ +670,3 @@
+ ///
+ /// /param S Associated statement.
+ ///
----------------
Dmitri Gribenko wrote:
> '\param' with backslash -- but you can drop the \param altogether, it is obvious from \brief.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:685
@@ +684,3 @@
+ ///
+ /// \brief Loc New starting location of directive.
+ ///
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:690
@@ +689,3 @@
+ ///
+ /// \brief Loc New ending location of directive.
+ ///
----------------
Wei Pan wrote:
> param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:701
@@ +700,3 @@
+ ///
+ OMPClause *getClause(unsigned i) {
+ assert(i < NumClauses && "Wrong number of clause!");
----------------
Dmitri Gribenko wrote:
> Do we need the non-const overload at all? It is identical to the const overload below.
Ok, will be removed
================
Comment at: include/clang/AST/StmtOpenMP.h:702
@@ +701,3 @@
+ OMPClause *getClause(unsigned i) {
+ assert(i < NumClauses && "Wrong number of clause!");
+ return getClauses()[i];
----------------
Wei Pan wrote:
> index out of bound?
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:727
@@ +726,3 @@
+
+ static bool classof(const Stmt *S) {
+ return S->getStmtClass() == OMPParallelDirectiveClass;
----------------
Wei Pan wrote:
> move the following two functions to the of the class. This is consistent with other statements.
I will change classoff() method for future use.
Method children() is going to be the same for all directives.
================
Comment at: include/clang/AST/StmtOpenMP.h:735
@@ +734,3 @@
+
+ ArrayRef<OMPClause *> clauses() {
+ return getClauses();
----------------
Dmitri Gribenko wrote:
> Drop the non-const overload?
>
> Also, why have this 'alias' for getClauses() anyway? The name is against coding standard.
Ok. Method will be renamed.
================
Comment at: include/clang/AST/StmtOpenMP.h:755
@@ +754,3 @@
+ ///
+ /// \param StartLoc Starting location of the directive kind.
+ /// \param EndLoc Ending Location of the directive.
----------------
Wei Pan wrote:
> it is the starting location of the directive not the directive kind?
It is the location of the directive keyword ('parallel', 'for' etc.)
================
Comment at: include/clang/Basic/OpenMPKinds.def:36
@@ +35,3 @@
+
+// OpenMP clause.
+OPENMP_CLAUSE(if, OMPIfClause)
----------------
Wei Pan wrote:
> clauses
Ok
================
Comment at: include/clang/Basic/OpenMPKinds.def:46
@@ +45,3 @@
+
+// Clauses allowed for OpenMP directives
+OPENMP_PARALLEL_CLAUSE(if)
----------------
Dmitri Gribenko wrote:
> '.' at the end.
Ok
================
Comment at: lib/AST/Stmt.cpp:1139
@@ +1138,3 @@
+StmtRange OMPClause::children() {
+ switch(getClauseKind()) {
+ default : break;
----------------
Dmitri Gribenko wrote:
> Space before '('
The method will be removed
================
Comment at: lib/AST/StmtPrinter.cpp:587
@@ +586,3 @@
+namespace {
+class OMPClausePrinter : public OMPClauseVisitor<OMPClausePrinter> {
+ StmtPrinter *Printer;
----------------
Dmitri Gribenko wrote:
> Make sure to add tests for printing when parsing and semantic analysis is implemented.
Of course, the tests for printing are developed already.
================
Comment at: lib/AST/StmtPrinter.cpp:665
@@ +664,3 @@
+ OS << (I == Node->varlist_begin() ? '(' : ',')
+ << *cast<NamedDecl>(cast<DeclRefExpr>(*I)->getDecl());
+ }
----------------
Wei Pan wrote:
> This needs to change, if it is a MemberExpr. The same as the others.
MemeberExpr is not allowed per standard
================
Comment at: lib/AST/StmtPrinter.cpp:705
@@ +704,3 @@
+ if (Node->getAssociatedStmt()) {
+ assert(isa<CompoundStmt>(Node->getAssociatedStmt()) &&
+ "Expected compound statement!");
----------------
Wei Pan wrote:
> Cannot be a for statement or AttributedStmt?
>
> #pragma omp firstprivate(a)
> [[some C++11 attributes]] for (int i = 0; i < 10; i++) {
> }
>
All statements are aggregated into CompoundStmt (for jump analysis). So the top statement should be a CompoundStmt.
================
Comment at: lib/Basic/OpenMPKinds.cpp:53
@@ +52,3 @@
+
+const char *clang::getOpenMPClauseName(OpenMPClauseKind Kind) {
+ assert(Kind < NUM_OPENMP_CLAUSES);
----------------
Doug Gregor wrote:
> This isn't actually used right now. What's it's purpose? It would be fine if it's part of debugging, but generally we avoid splicing strings like this into diagnostics (for example).
It is used for diagnostic.
For example, if the variable is marked as private, it cannot be shared. So, in this case a message is generated:
"%0 variable cannot be %1", where %0 and %1 are generated by getOpenMPClauseName(CurrentVarKind) and getOpenMPClauseName(CurrentClauseKind).
================
Comment at: lib/Basic/OpenMPKinds.cpp:63-64
@@ +62,4 @@
+ return "threadprivate or thread local";
+ default:
+ break;
+ }
----------------
Dmitri Gribenko wrote:
> 'default' should not be needed -- here and in other functions in this file.
There is NUM_OPENMP_CLAUSES, which is not covered. Should I include it explicitly into the switch?
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1616-1617
@@ +1615,4 @@
+ break;
+ default:
+ return 0;
+ }
----------------
Dmitri Gribenko wrote:
> Is this possible at all?
No, I'll remove it.
================
Comment at: tools/libclang/CIndex.cpp:1920
@@ +1919,3 @@
+ unsigned size = WL.size();
+ for (Stmt::const_child_range Child = S->children(); Child; ++Child) {
+ AddStmt(*Child);
----------------
Wei Pan wrote:
> no {
>
> and
>
> }
Ok
http://llvm-reviews.chandlerc.com/D572
More information about the cfe-commits
mailing list