[PATCH] OpenMP parallel directive and qualified names in threadprivate

[Alexey Bataev]

  Doug, I'm sorry for such big and complex patch. I understand that is very problematic for you, Dmitri, Hal and others to review such large patches. But currently it's quite hard to separate this patch and the next one (with '#pragma omp for' support, which is smaller, but still quite big) into several small patches. I'll try to make next patches much smaller and more convenient for review.


================
Comment at: include/clang/AST/StmtOpenMP.h:72
@@ +71,3 @@
+/// clauses in the '#pragma omp ...' directive.
+class OMPSingleExpr {
+  /// \brief Expression for the clause.
----------------
Doug Gregor wrote:
> This base class just doesn't seem worth it. It's only abstracting over the storage for a single expression, which I'd rather just see in the various subclasses of OMPSingleExpr. Plus, this abstraction has caused the weird getExpr() signature, which returns the address of the underlying expression rather than just the expression.
> 
> Conventionally, we'd store this as a Stmt* and cast to/from Expr* in the accessor and mutator.
Doug, Ok, I'll remove it.

================
Comment at: include/clang/AST/StmtOpenMP.h:94
@@ +93,3 @@
+template <typename T>
+class OMPSimple {
+  /// \brief Argument of the clause.
----------------
Doug Gregor wrote:
> Similar to OMPSingleExpr, this just doesn't seem worth it, and it's an odd use of multiple inheritance.
> 
> Plus, it means that all of the subclasses have the generic name "getArgument()" for their accessor, as well as having a mutator they don't need. We should have more descriptive accessor names, and mutation (which only happens in the ASTReader) should be handled directly by friend'ing the reader.
Ok

================
Comment at: include/clang/AST/StmtOpenMP.h:132
@@ +131,3 @@
+  /// \brief List of references to variables.
+  Expr **Vars;
+protected:
----------------
Doug Gregor wrote:
> Part of the reason for tail-allocating the list of variable references is to eliminate this pointer from the AST. If we're going to inherit from OMPVarList to get this information, I suggest templating it on the subclass, so we can downcast to that class and use "this+1" as the address of the list of variable references.
You're right. That's the better solution.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6216
@@ +6215,3 @@
+def err_omp_private_incomplete_type : Error<
+  "a private variable must not have incomplete type %0">;
+def err_omp_firstprivate_incomplete_type : Error<
----------------
Doug Gregor wrote:
> How about simple "private variable with incomplete type %0", like we do elsewhere in Clang? The same comment for the next few diagnostics.
Ok.

================
Comment at: lib/Parse/ParseOpenMP.cpp:71
@@ +70,3 @@
+///       threadprivate-directive
+///         annot_pragma_openmp threadprivate simple-variable-list
+///         annot_pragma_openmp_end
----------------
Doug Gregor wrote:
> threadprivate is a keyword, so it should be in 'single quotes'
Ok, I'll fix all comments

================
Comment at: lib/Parse/ParseOpenMP.cpp:138
@@ +137,3 @@
+        Stmt *AStmt = AssociatedStmt.take();
+        AssociatedStmt = Actions.ActOnCompoundStmt(AStmt->getLocStart(),
+                                                   AStmt->getLocEnd(),
----------------
Doug Gregor wrote:
> If we're just parsing one statement, why are we wrapping it up in a CompoundStmt?
Actually, I want to use the same code for all directives. By there is at least one directive (#pragma omp for), which could have several associated for-loops. For example:
#pragma omp for collapse(2)
for(..) {
}
for(...) {
}
These 2 loops will be associated with the '#pragma omp for'.

================
Comment at: lib/Parse/ParseOpenMP.cpp:258
@@ +257,3 @@
+      << getOpenMPClauseName(CKind) << getOpenMPDirectiveName(DKind);
+    SkipUntil(tok::comma, tok::annot_pragma_openmp_end, false, true);
+    return StmtError();
----------------
Doug Gregor wrote:
> This recovery seems overly aggressive. Why not still parse the clause, but have the caller ignore it?
Ok. I'll fix it.

================
Comment at: lib/Parse/ParseOpenMP.cpp:372
@@ +371,3 @@
+
+  if (LParen && Tok.isNot(tok::r_paren)) {
+    Diag(Tok, diag::err_expected_rparen);
----------------
Doug Gregor wrote:
> You can use the BalancedDelimiterTracker to automate some of these diagnostics.
Nope, I can't. Actually I tried to use it. It uses ';' as the stop point for parsing, but I have to stop parsing of the directive at 'tok::annot_pragma_openmp_end'. This class makes parsing very complex, because it skips all annotation tokens for OpenMP directives.

================
Comment at: lib/Sema/SemaOpenMP.cpp:34
@@ +33,3 @@
+
+static bool isConstant(ASTContext &Context, QualType Type) {
+  CanQualType CT = Context.getCanonicalType(nonRefType(Type));
----------------
Doug Gregor wrote:
> Please use QualType::isContext() directly.
Ok

================
Comment at: include/clang/AST/StmtOpenMP.h:59
@@ +58,3 @@
+  static bool classof(const Stmt *T) {
+    return T->getStmtClass() == OMPNumThreadsClauseClass ||
+           T->getStmtClass() == OMPIfClauseClass ||
----------------
Doug Gregor wrote:
> Can we use a more-efficient >= && <= comparison here?
I'll try to fix it.

================
Comment at: include/clang/AST/StmtOpenMP.h:31
@@ +30,3 @@
+///
+class OMPClause : public Stmt {
+  /// \brief Starting location of the clause.
----------------
Doug Gregor wrote:
> Why is an OMPClause a Stmt? These aren't executable code of any kind, nor are they expressions in the C sense. It makes sense to have an OMPClause and its subclasses, but I think it needs to be a standalone hierarchy. That means a little more work in the RecursiveASTVisitor, but it's the right solution.
Ok, I'll fix it.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6201
@@ -6200,3 +6200,3 @@
 // OpenMP support.
 def err_omp_expected_var_arg_suggest : Error<
   "%0 is not a global variable, static local variable or static data member%select{|; did you mean %2?}1">;
----------------
Doug Gregor wrote:
> Aside: can we introduce a new category for OpenMP diagnostics?
I'll try to make it.

================
Comment at: lib/Parse/ParseOpenMP.cpp:70
@@ +69,3 @@
+///
+///       threadprivate-directive
+///         annot_pragma_openmp threadprivate simple-variable-list
----------------
Doug Gregor wrote:
> Technically missing a ':' after threadprivate-directive
Ok.

================
Comment at: lib/Parse/ParseOpenMP.cpp:122
@@ +121,3 @@
+      // Skip ',' if any
+      if (Tok.is(tok::comma))
+        ConsumeToken();
----------------
Doug Gregor wrote:
> Are the commas actually optional?
Yes, commas are optional. For example, here is the syntax of #pragma omp parallel from standard document:
#pragma omp parallel [clause[ [, ]clause] ...] new-line
As you can see, comma is optional.

================
Comment at: lib/Sema/SemaOpenMP.cpp:30
@@ -22,1 +29,3 @@
 
+static QualType nonRefType(QualType Type) {
+  return Type.getNonReferenceType();
----------------
Doug Gregor wrote:
> Please don't add this. Use QualType::getNonReferenceType() directly.
Ok.


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