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

[Alexey Bataev]

John, thanks for the review!


================
Comment at: include/clang/AST/OpenMPClause.h:1156
@@ -1140,2 +1155,3 @@
+/// clause.
 ///
 class OMPLastprivateClause : public OMPVarListClause<OMPLastprivateClause> {
----------------
rjmccall wrote:
> 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. :)
Ok, Done.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:275
@@ +274,3 @@
+                          CapturedStmtInfo->lookup(OrigVD) != nullptr,
+                          (*IRef)->getType(), VK_LValue, (*IRef)->getExprLoc());
+          return EmitLValue(&DRE).getAddress();
----------------
rjmccall wrote:
> Is this DRE actually different from *IDestRef?
Yes, it is different. IDestRef points to some pseudo-variable, but we need it to point to the address of the original variable.

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

================
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());
----------------
rjmccall wrote:
> 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.
John, thanks for pointing out to this important problem! I will fix it ASAP.

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

================
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);
----------------
rjmccall wrote:
> Spacing.  Also, s/was/were/ in the comment.
Fixed

http://reviews.llvm.org/D8658

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