Page MenuHomePhabricator

[OMPT] Fix OMPT callbacks for the taskloop construct and add testcase
ClosedPublic

Authored by sconvent on Jun 4 2018, 5:41 AM.

Details

Summary

Fix the order of callbacks related to the taskloop construct.
Add the iteration_count to work callbacks (according to the spec).
Use kmpc_omp_task() instead of kmp_omp_task() to include OMPT callbacks.
Add a testcase.

Diff Detail

Repository
rL LLVM

Event Timeline

sconvent created this revision.Jun 4 2018, 5:41 AM
sconvent updated this revision to Diff 149754.Jun 4 2018, 6:55 AM
sconvent edited the summary of this revision. (Show Details)

Use kmpc_omp_task() instead of changing kmp_omp_task().

protze.joachim added inline comments.Jun 4 2018, 1:40 PM
runtime/src/kmp_tasking.cpp
3754 ↗(On Diff #149754)

same as below

3910 ↗(On Diff #149754)

We somehow need to pass a return-address in here. So probably a new variant of __kmpc_omp_task, which takes the address and adds it to the ompt-callback invocation?

3955 ↗(On Diff #149754)

Shouldn't there be st(ride) in the formula?

AndreyChurbanov added inline comments.
runtime/src/kmp_tasking.cpp
3955 ↗(On Diff #149754)

Why not use calculated below iterations count - tc? It is computed at lines 3980 - 3995, including the check against zero.

sconvent added inline comments.Jun 6 2018, 5:30 AM
runtime/src/kmp_tasking.cpp
3955 ↗(On Diff #149754)

There is a difference between the iteration count and the task count.
From the spec (TR6 page 423):

For a taskloop construct, count represents the number of iterations in the iteration space, which may be the result of collapsing several associated loops.

tc seems to be the task count, not the iteration count. And st(ride) is not needed for the calculation of the iteration count, right?

AndreyChurbanov added inline comments.Jun 6 2018, 5:48 AM
runtime/src/kmp_tasking.cpp
3955 ↗(On Diff #149754)

No, the tc states for "trace count" which is exactly iterations count. And the stride is important here.
E.g. for(i=0; i<10; i+=3) causes the iteration count to be 4 not 10, because of stride=3.
For task count another parameter used - it is grainsize.

sconvent updated this revision to Diff 150119.Jun 6 2018, 6:04 AM

Use tc for the iteration count

runtime/src/kmp_tasking.cpp
3955 ↗(On Diff #149754)

You're right, I updated the diff to use tc.

Use tc for the iteration count

Can you add another loop in the test case where this makes a difference? Maybe just Andrey's example

sconvent updated this revision to Diff 150122.Jun 6 2018, 6:14 AM

Use a stride of 3 in testcase loop

taskloop is actually a tasking construct and explicitly no worksharing construct. So, please move the test in the task directory.

sconvent updated this revision to Diff 150719.Jun 11 2018, 5:41 AM

Add support for OMPT return addresses.
Also test them.

taskloop is actually a tasking construct and explicitly no worksharing construct. So, please move the test in the task directory.

Done

hbae added inline comments.Jul 19 2018, 2:50 PM
runtime/src/kmp_tasking.cpp
1602 ↗(On Diff #150719)

Can we change the function name to __kmp_omp_taskloop_task?
__kmpc_* is usually for ABI functions.

4005 ↗(On Diff #150719)

I think it is better to keep the original code (__kmp_omp_task() call) at this line.

sconvent marked 7 inline comments as done.Jul 26 2018, 1:55 AM
protze.joachim accepted this revision.Jul 27 2018, 10:41 AM

I apply changes requested by hbae on commit.

LGTM

This revision is now accepted and ready to land.Jul 27 2018, 10:41 AM
This revision was automatically updated to reflect the committed changes.
Hahnfeld added inline comments.
openmp/trunk/runtime/src/kmp_tasking.cpp
4018

This call wasn't correctly reverted prior to commit, @gtbercea is fixing this in D50001