Page MenuHomePhabricator

[OMPT] Add interoperability testcase
ClosedPublic

Authored by sconvent on Jan 11 2018, 2:52 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sconvent created this revision.Jan 11 2018, 2:52 AM
protze.joachim added inline comments.Jan 11 2018, 4:18 AM
runtime/test/ompt/misc/interoperability.cpp
1 ↗(On Diff #129421)

I think, this is not needed for this case. Just run the sorted checks.

  • Remove the first line
  • Replace THREADS by CHECK
sconvent updated this revision to Diff 130151.Jan 17 2018, 5:35 AM

Implemented suggestions

sconvent marked an inline comment as done.Jan 17 2018, 5:36 AM
hbae accepted this revision.Feb 7 2018, 11:55 AM

LGTM.

This revision is now accepted and ready to land.Feb 7 2018, 11:55 AM
omalyshe requested changes to this revision.Feb 8 2018, 1:09 AM
omalyshe added inline comments.
runtime/test/ompt/misc/interoperability.cpp
39 ↗(On Diff #130151)

These three checks relates to the second master thread, so to make it clear I'd suggest naming it MASTER_ID_2.

46 ↗(On Diff #130151)

Two checks above should has different thread ids, that is THREAD_ID_1, THREAD_ID_2 (something like that).

I also suggest adding CHECK for *_end events for all *_begin events above.

This revision now requires changes to proceed.Feb 8 2018, 1:09 AM

I think if you re-capture a variable with FileCheck that's the same as capturing another variable except that you can't use the old value. So I'm not sure if the changes make any difference, besides naming...

I think if you re-capture a variable with FileCheck that's the same as capturing another variable except that you can't use the old value. So I'm not sure if the changes make any difference, besides naming...

Yes, I understand that. But it is difficult to understand what the test is actually checking due to no comments and same identifiers usage.

sconvent updated this revision to Diff 134182.Feb 14 2018, 2:54 AM

Implemented suggestions

sconvent marked 2 inline comments as done.Feb 14 2018, 2:55 AM

Implemented changes look good.

However, it's probably worth to apply re-formatting suggested by @Hahnfeld in https://reviews.llvm.org/D43191.

sconvent updated this revision to Diff 134398.Feb 15 2018, 3:24 AM

Add some comments to checks and reformat as suggested

omalyshe accepted this revision.Feb 15 2018, 3:36 AM

LGTM.

This revision is now accepted and ready to land.Feb 15 2018, 3:36 AM

Add some comments to checks and reformat as suggested

Could you please reformat the full file with clang-format? The indention doesn't look right...

I applied clang-format on commit.

Having the tests after the end of the main function gives us actually 2 more characters :)

Closed by commit rL325423: [OMPT] Add interoperability testcase (authored by jprotze, committed by ). · Explain WhyFeb 17 2018, 1:45 AM
This revision was automatically updated to reflect the committed changes.

This test doesn't work if threads are not scheduled as expected: http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/58
I think in that case a worker thread started before the second master which makes sense as there is no synchronization between the two initial threads. We should be able to fix this by either starting a serialized region or calling an API function and putting an OMPT_WAIT before the real parallel region. Thoughts?

Btw: What do you need the OMPT_WAIT(condition, 6) for?

The idea was that the WAIT in line 11 should ensure that both initial threads arrived.
But actually the runtime is not initialized before line 11. To fix the race, we need to call into the runtime in a way, that makes both threads initial threads before the SIGNAL in line 9.

The WAIT in line 16 is to ensure that both worker threads are recruited separately. Not sure whether it would be possible that the same thread is recruited by both initial threads.