This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Initial testsuite change to purely llvm-lit based testing
ClosedPublic

Authored by jlpeyton on Aug 6 2015, 3:16 PM.

Details

Summary

This change introduces a check-libomp target which is based upon llvm's lit test infrastructure. Each test (generated from the University of Houston's OpenMP testsuite) is compiled and then run. For each test, an exit status of 0 indicates success and non-zero indicates failure. This way, FileCheck is not needed. I've added a bit of logic to generate symlinks (libiomp5 and libgomp) in the build tree so that gcc can be tested as well.

Diff Detail

Repository
rL LLVM

Event Timeline

jlpeyton updated this revision to Diff 31479.Aug 6 2015, 3:16 PM
jlpeyton retitled this revision from to [OpenMP] Initial testsuite change to purely llvm-lit based testing.
jlpeyton updated this object.
jlpeyton added a reviewer: hfinkel.
jlpeyton set the repository for this revision to rL LLVM.
jlpeyton added a subscriber: openmp-commits.
gribozavr added inline comments.
runtime/test/api/has_openmp.c
1 ↗(On Diff #31479)

Instead of duplicating the RUN line everywhere, I'd suggest to introduce a substitution, say, %omp-run-simple, that would be used in all "typical" tests. Then, the tests that need special handling will have a custom run line.

jlpeyton updated this revision to Diff 31526.Aug 7 2015, 10:05 AM

Addressing Dmitri Gribenko's suggestion by creating %libomp-compile-and-run substitution.

gribozavr added inline comments.Aug 9 2015, 11:49 AM
runtime/test/lit.cfg
53 ↗(On Diff #31526)

You'd need a space between the compiler and the flags.

Also, why not use the %clang and %cflags in the %libomp-compile-and-run substitution?

jlpeyton updated this revision to Diff 31673.Aug 10 2015, 8:43 AM

You'd need a space between the compiler and the flags.

There was a prepended space in the config.test_cflags variable, but I have moved it to make it clearer.

Also, why not use the %clang and %cflags in the %libomp-compile-and-run substitution?

I could not get it to substitute the values. llvm-lit would try to execute '%clang' which of course failed, and %cflags would not substitute either. Fortunately, %t and %s did substitute like normal.

I could not get it to substitute the values. llvm-lit would try to execute '%clang' which of course failed, and %cflags would not substitute either. Fortunately, %t and %s did substitute like normal.

I think that's because lit applies substitutions first-to-last. Try putting %clang and %cflags last.

jlpeyton updated this revision to Diff 31690.Aug 10 2015, 10:51 AM

I think that's because lit applies substitutions first-to-last. Try putting %clang and %cflags last.

That did the trick, thanks!

hfinkel edited edge metadata.Aug 13 2015, 4:52 PM

I see code in the lit/cmake files to handle windows, does this currently work? The tests seem to include unistd.h.

I see code in the lit/cmake files to handle windows, does this currently work? The tests seem to include unistd.h.

Currently, it is Unices and Mac only. I only put the Windows logic in there in anticipation that it will be supported at some point. Right now, users can't easily use libomp.so on Windows because the clang front end doesn't include it when using -fopenmp (libomp.lib has to be explicitly linked).

I see code in the lit/cmake files to handle windows, does this currently work? The tests seem to include unistd.h.

Currently, it is Unices and Mac only. I only put the Windows logic in there in anticipation that it will be supported at some point. Right now, users can't easily use libomp.so on Windows because the clang front end doesn't include it when using -fopenmp (libomp.lib has to be explicitly linked).

Okay (although there is a patch out to fix the libomp.lib problem, the larger problem is that the tests themselves won't work because of the unistd.h dependency). Although we normally frown on committing dead code, in this particular case, the couple of lines seems straightforward, so I don't have a strong opinion. We should probably have it explicitly fail on Windows, however, with some informative 'Regression tests not supported yet' message instead of having the user run all of the tests and having them all appear to fail. We can remove this once we actually update the tests to compile on Windows.

(I've also added Chandler to the review; he told me he'd look at this soon).

jlpeyton updated this revision to Diff 32452.Aug 18 2015, 2:42 PM

Okay (although there is a patch out to fix the libomp.lib problem, the larger problem is that the tests themselves won't work because of the unistd.h dependency)

I've rid the testsuite of unistd.h (which isn't needed) and added another my_sleep() function which is Windows compatible.

hfinkel added inline comments.Aug 29 2015, 11:55 AM
runtime/test/omp_my_sleep.h
13 ↗(On Diff #32452)

Why do we need windows.h here? clock() is in time.h.

53 ↗(On Diff #32452)

I don't understand why we're sleeping this way -- it is not really sleeping, but rather busy-waiting on the clock. Shouldn't we use something like nanosleep() instead?

hfinkel added inline comments.Aug 29 2015, 12:16 PM
runtime/test/atomic/omp_atomic.c
5 ↗(On Diff #32452)

my_sleep is not called in this file.

runtime/test/critical/omp_critical.c
4 ↗(On Diff #32452)

my_sleep is not called in this file.

runtime/test/tasking/omp_task_if.c
22 ↗(On Diff #32452)

Please remove commented-out code.

26 ↗(On Diff #32452)

Here too (or maybe this shouldn't be commented?)

runtime/test/worksharing/for/omp_for_schedule_dynamic.c
15 ↗(On Diff #32452)

my_sleep is not called in this file.

jlpeyton updated this revision to Diff 33716.Sep 1 2015, 11:45 AM

Why do we need windows.h here? clock() is in time.h.
I don't understand why we're sleeping this way -- it is not really sleeping, but rather busy-waiting on the clock. Shouldn't we use something like nanosleep() instead?

I've changed the my_sleep() function to actually sleep instead of busy wait. Windows uses Sleep() (which requires windows.h now) and Unices will use nanosleep().

my_sleep is not called in this file.

Removed.

my_sleep is not called in this file.

Removed.

Please remove commented-out code.
Here too (or maybe this shouldn't be commented?)

Changed to critical section

my_sleep is not called in this file.

Removed.

hfinkel accepted this revision.Sep 15 2015, 5:55 AM
hfinkel edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Sep 15 2015, 5:55 AM
This revision was automatically updated to reflect the committed changes.

Can we get a back port of Commit rL248211 to the openmp 3.7 release branch for the 3.7.1 release? It applies cleanly here over openmp 3.7.0 and shows perfect results...

Testing Time: 27.55s

Expected Passes    : 66

[100%] Built target check-libomp

against clang 3.7.0.

There is one issue with rL248211. On darwin, the check-libomp test suite run depends on DYLD_LIBRARY_PATH being passed along through make. This isn't true in El Capitan due to the System Integrity Protection pruning of DYLD_LIBRARY_PATH.

https://developer.apple.com/library/prerelease/ios/documentation/Security/Conceptual/System_Integrity_Protection_Guide/System_Integrity_Protection_Guide.pdf

The fix is to modify the cmake build such that "-Wl,-rpath,$libomp_path" is appended to LIBOMP_TEST_CFLAGS on darwin.

One other issue. It appears that the tolerances used in runtime/test/api/omp_get_wtime.c are too tight for OS X 10.11. With or without SIP enabled, I get a return code between 2 to 6 on that test. Opening it up to...

return ((measured_time > 0.98 * wait_time) && (measured_time < 1.02 * wait_time)) ;

has the return code oscillate between returning 0 or 1.

return ((measured_time > 0.97 * wait_time) && (measured_time < 1.03 * wait_time)) ;

on my MacPro 3,1 dual quad-core is required under 10.11 to have the return code always report back as 0.