[PATCH] [OPENMP] Codegen for 'lastprivate' clause in 'for' directive.

[John McCall]

Generally looks good.  A few editorial comments and a larger point about declaration identity.


================
Comment at: include/clang/AST/OpenMPClause.h:1156
@@ -1140,2 +1155,3 @@
+/// clause.
 ///
 class OMPLastprivateClause : public OMPVarListClause<OMPLastprivateClause> {
----------------
This isn't external documentation, it's implementation documentation, so it should be written with // instead of /// and it should go inside the class definition.  But otherwise, thank you for writing it. :)

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:275
@@ +274,3 @@
+                          CapturedStmtInfo->lookup(OrigVD) != nullptr,
+                          (*IRef)->getType(), VK_LValue, (*IRef)->getExprLoc());
+          return EmitLValue(&DRE).getAddress();
----------------
Is this DRE actually different from *IDestRef?

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:281
@@ +280,3 @@
+        // generated. Initialization of this variable will happen in codegen for
+        // 'firstprivate' clause.
+        if (!IInit)
----------------
Word wrap.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:327
@@ +326,3 @@
+        if (AlreadyEmittedVars.count(PrivateVD) == 0) {
+          AlreadyEmittedVars.insert(PrivateVD);
+          auto *SrcVD = cast<VarDecl>(cast<DeclRefExpr>(*ISrcRef)->getDecl());
----------------
This is:
  if (AlreadyEmittedVars.insert(PrivateVD).second) {

Also, in general, you should only ever use canonical declarations as hash keys.  The only time that's unnecessary is when you know you're dealing with something that can't be redeclared (e.g. a non-extern local variable or a non-static data member).

In principle, it would also be unnecessary whenever it's structurally impossible for different places to see different declarations.  For example, in C you don't need to canonicalize here because (1) the lastprivate clauses have to be adjacent without any intervening declarations and (2) they must be written in exactly the same way, and so (3) they perform the exact same lookup and must find the exact same redeclaration.  But in C++, differently-qualified names can find different redeclarations because of using declarations, so you really do need to always canonicalize.  For example, this would do it:
  namespace A { int x; }
  namespace B { using A::x; }
  namespace A { int x = 4; }
...
  #pragma omp for lastprivate(A::x, B::x)

Plus, who knows, somebody might add some crazy language extension which basically recreates this problem in C (I could easily imagine it coming up with modules, for example).  So it's safest to just always use canonical declarations as keys.

This is something you need to worry about in Sema, too; it looks like getTopDSA has the same problem, where you could theoretically apply inconsistent rules to the same entity using different redeclarations.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:902
@@ -807,3 +901,3 @@
 
-void CodeGenFunction::EmitOMPWorksharingLoop(const OMPLoopDirective &S) {
+bool CodeGenFunction::EmitOMPWorksharingLoop(const OMPLoopDirective &S) {
   // Emit the loop iteration variable.
----------------
.

================
Comment at: lib/Sema/SemaOpenMP.cpp:595
@@ +594,3 @@
+            // variable for
+            // proper diagnostics.
+            auto *VDPrivate = VarDecl::Create(
----------------
Word wrap.

================
Comment at: lib/Sema/SemaOpenMP.cpp:615
@@ +614,3 @@
+        // Set initializers to private copies if no errors was found.
+        if (PrivateCopies.size() ==  Clause->varlist_size()) {
+          Clause->setPrivateCopies(PrivateCopies);
----------------
Spacing.  Also, s/was/were/ in the comment.

http://reviews.llvm.org/D8658

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/