This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ABataev on Mar 27 2015, 5:04 AM.

Details

Summary
#pragma omp for lastprivate(<var>)
for (i = a; i < b; ++b)
<BODY>;

This construct is translated into something like:

<last_iter> = alloca i32
<lastprivate_var> = alloca <type>
<last_iter> = 0
; No initializer for simple variables or a default constructor is called for objects.
; For arrays perform element by element initialization by the call of the default constructor.
...
OMP_FOR_START(...,<last_iter>, ..); sets <last_iter> to 1 if this is the last iteration.
<BODY>
...
OMP_FOR_END
if (<last_iter> != 0) {
<var> = <lastprivate_var> ; Update original variable with the lastprivate value.
}
call __kmpc_cancel_barrier() ; an implicit barrier to avoid possible data race.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 22785.Mar 27 2015, 5:04 AM
ABataev retitled this revision from to [OPENMP] Codegen for 'lastprivate' clause in 'for' directive..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
ABataev updated this revision to Diff 23479.Apr 9 2015, 6:39 AM

Updated to meet latest changes in patches for 'copyprivate' clause and new one for 'copyin' clause.

ABataev updated this revision to Diff 23588.Apr 10 2015, 5:12 AM

Updated to the latest changes in OpenMP API.

rjmccall edited edge metadata.Apr 10 2015, 10:30 AM

This patch seems to have the aggregate-copy changes still in it, would you mind rebasing it without them?

include/clang/AST/OpenMPClause.h
1171 ↗(On Diff #23588)

As always, when you've got tail-allocated arrays like this, please leave a comment describing their layout somewhere near the top of the class body.

ABataev updated this revision to Diff 23717.Apr 13 2015, 11:19 PM
ABataev edited edge metadata.

Updated after review

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

include/clang/AST/OpenMPClause.h
1156 ↗(On Diff #23717)

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. :)

lib/CodeGen/CGStmtOpenMP.cpp
275 ↗(On Diff #23717)

Is this DRE actually different from *IDestRef?

281 ↗(On Diff #23717)

Word wrap.

327 ↗(On Diff #23717)

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.

902 ↗(On Diff #23717)

.

lib/Sema/SemaOpenMP.cpp
595 ↗(On Diff #23717)

Word wrap.

615 ↗(On Diff #23717)

Spacing. Also, s/was/were/ in the comment.

John, thanks for the review!

include/clang/AST/OpenMPClause.h
1156 ↗(On Diff #23717)

Ok, Done.

lib/CodeGen/CGStmtOpenMP.cpp
275 ↗(On Diff #23717)

Yes, it is different. IDestRef points to some pseudo-variable, but we need it to point to the address of the original variable.

281 ↗(On Diff #23717)

Arghhh, automatic reformatting, thanks.

327 ↗(On Diff #23717)

John, thanks for pointing out to this important problem! I will fix it ASAP.

lib/Sema/SemaOpenMP.cpp
595 ↗(On Diff #23717)

Thanks, fixed

615 ↗(On Diff #23717)

Fixed

ABataev updated this revision to Diff 23760.Apr 15 2015, 2:35 AM

Fixed after review

LGTM. Please do add a test for the redeclaration problem, though; you can do that without review. Also, don't forget to fix it in Sema as well.

John, Ok, I'll add a test. Of course, fix for Sema will include tests
for this problem.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

15.04.2015 21:25, John McCall пишет:

LGTM. Please do add a test for the redeclaration problem, though; you can do that without review. Also, don't forget to fix it in Sema as well.

http://reviews.llvm.org/D8658

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
This revision was automatically updated to reflect the committed changes.