This is an archive of the discontinued LLVM Phabricator instance.

Fix omp_sections_nowait.c test to address Bugzilla Bug 28336
ClosedPublic

Authored by jlpeyton on Jun 29 2016, 10:11 AM.

Details

Summary

This rewrite of the omp_sections_nowait.c test file causes it to hang if the nowait is not respected. If the nowait isn't respected, the lone thread which can escape the first sections construct will just sleep at a barrier which shouldn't exist. All reliance on timers is taken out. For good measure, the test makes sure that all eight sections are executed as well. The test should take no longer than a few seconds on any modern machine.

Diff Detail

Repository
rL LLVM

Event Timeline

jlpeyton updated this revision to Diff 62234.Jun 29 2016, 10:11 AM
jlpeyton retitled this revision from to Fix omp_sections_nowait.c test to address Bugzilla Bug 28336.
jlpeyton updated this object.
jlpeyton added a reviewer: hfinkel.
jlpeyton set the repository for this revision to rL LLVM.
jlpeyton added subscribers: openmp-commits, ABataev.
hfinkel accepted this revision.Jun 29 2016, 10:24 AM
hfinkel edited edge metadata.

Thanks! LGTM.

test/worksharing/sections/omp_sections_nowait.c
23 ↗(On Diff #62234)

I don't recall how this part of OpenMP's memory model is specified. Do you technically need an omp flush(release) here too to force this thread to synchronize its local memory view with the global one?

This revision is now accepted and ready to land.Jun 29 2016, 10:24 AM
AndreyChurbanov added inline comments.
test/worksharing/sections/omp_sections_nowait.c
23 ↗(On Diff #62234)

In theory all accesses to "release" variable are data races here. And flush won't change this. So theoretically atomic reads/writes could help the test be absolutely correct.

But in practice these data races are benign, so I think it is OK to rely on compiler to honor volatile keyword and the fact that int type has all reads and writes atomic (unless you artificially align it on odd boundary).

So I would not introduce complication with omp atomic read/write for variable "release" now.

Just my 2 cents.

hfinkel added inline comments.Jun 29 2016, 11:05 AM
test/worksharing/sections/omp_sections_nowait.c
23 ↗(On Diff #62234)

Fair enough; fine by me. I'm fine with our test suite testing non-standard guarantees we expect our implementation (and any reasonable implementation) to provide.

This revision was automatically updated to reflect the committed changes.