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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
