Page MenuHomePhabricator

[OPENMP] Codegen for 'omp barrier' directive.
ClosedPublic

Authored by ABataev on Nov 28 2014, 4:44 AM.

Details

Summary

Adds generation of call to "i32 kmpc_cancel_barrier(ident_t *, i32)" libcall for explicitly specified barriers (OMP_IDENT_BARRIER_EXPL flag is added to "flags" field of "ident_t" structure).
Also this patch replaces all calls to "
kmpc_barrier" function by calls of "__kmpc_cancel_barrier" function which provides additional functionality for OpenMP 4.0.
Also, library specific enum OpenMPLocationFlags moved to private section of CGOpenMPRuntime class to make it more independent from library implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 16723.Nov 28 2014, 4:44 AM
ABataev retitled this revision from to [OPENMP] Codegen for 'omp barrier' directive..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
rjmccall edited edge metadata.Dec 1 2014, 4:48 PM

I'd appreciate it if you described the rationale for changes like this somewhere in the code. I'm happy to take your word for it that this is correct, but it will help later people looking at this code if they can see why you're using one function instead of the other.

lib/CodeGen/CGOpenMPRuntime.cpp
640 ↗(On Diff #16723)

Does this runtime function always exist, or is it contingent on having a sufficiently new OpenMP runtime? If the latter, you should introduce the concept of a target OpenMP runtime version, patterned on the target Objective-C runtime version.

Also, please describe in a comment why we're using this function rather than the other, since apparently both "work" in some sense.

lib/CodeGen/CGOpenMPRuntime.h
96 ↗(On Diff #16723)

Is this supposed to be the same value as OMP_IDENT_BARRIER_IMPL? If so, is there a better way to define these constants that makes their structure more obvious?

238 ↗(On Diff #16723)

Don't make two functions with the same name that differ only in an enum vs. bool argument.

ABataev added inline comments.Dec 3 2014, 12:41 AM
lib/CodeGen/CGOpenMPRuntime.cpp
640 ↗(On Diff #16723)

Yes, it exists always. In OpenMP 3.1 it works just the same way as a regular kmpc_barrier() (actually, kmpc_cancel_barrier just calls it). But it adds processing of cancellation directives introduced in OpenMP 4.0.
Ok, I'll add the comment.

lib/CodeGen/CGOpenMPRuntime.h
96 ↗(On Diff #16723)

Yes, these values are just copied from runtime header file. We need to modify runtime to modify these values

238 ↗(On Diff #16723)

Ok, I'll rename one of them

ABataev updated this revision to Diff 16858.Dec 3 2014, 3:19 AM
ABataev edited edge metadata.

Update after review

rjmccall added inline comments.Dec 3 2014, 12:12 PM
lib/CodeGen/CGOpenMPRuntime.cpp
640 ↗(On Diff #16723)

Okay. Is that desired for the existing uses of EmitOMPBarrierCall? If everybody always wants a cancel barrier, and both runtime functions take the same arguments, why are there two runtime functions at all?

John, it is for compatibility. We have to use kmp_cancel_barrier()
(we're going to implement full OpenMP 4.0), but there can be some old
solutions that can use old _kmpc_barrier() function.
Actually, I was mistaken when implemented 'omp barrier' using legacy
kmpc_barrier() instead of the new one kmpc_cancel_barrier() and this
patch just fixes it.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

03.12.2014 23:12, John McCall пишет:

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:640
@@ -642,3 +639,3 @@

OpenMPLocationFlags Flags) {
  • Build call kmpc_barrier(loc, thread_id) + Build call kmpc_cancel_barrier(loc, thread_id); llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc, Flags), ---------------- ABataev wrote:

rjmccall wrote:

Does this runtime function always exist, or is it contingent on having a sufficiently new OpenMP runtime? If the latter, you should introduce the concept of a target OpenMP runtime version, patterned on the target Objective-C runtime version.

Also, please describe in a comment why we're using this function rather than the other, since apparently both "work" in some sense.

Yes, it exists always. In OpenMP 3.1 it works just the same way as a regular kmpc_barrier() (actually, kmpc_cancel_barrier just calls it). But it adds processing of cancellation directives introduced in OpenMP 4.0.
Ok, I'll add the comment.

Okay. Is that desired for the existing uses of EmitOMPBarrierCall? If everybody always wants a cancel barrier, and both runtime functions take the same arguments, why are there two runtime functions at all?

http://reviews.llvm.org/D6447

Okays. Sound fine, then.

ABataev closed this revision.Dec 4 2014, 8:10 PM
ABataev updated this revision to Diff 16979.

Closed by commit rL223444 (authored by @ABataev).