This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][OpenMP] Enable Libomptarget runtime tests
ClosedPublic

Authored by pdhaliwal on Mar 31 2021, 5:19 AM.

Details

Summary

This enables the runtime tests on amdgpu targets.
10 tests have been marked as XFAIL on amdgcn currently mostly due to
missing printf.

Diff Detail

Event Timeline

pdhaliwal created this revision.Mar 31 2021, 5:19 AM
pdhaliwal requested review of this revision.Mar 31 2021, 5:19 AM
Herald added a project: Restricted Project. · View Herald Transcript

I guess there's no XFAIL equivalent here? In that case we should probably leave off the RUN line for the tests that can't work, as they'll otherwise break the build once an amdgpu CI machine goes live

openmp/libomptarget/CMakeLists.txt
40

I don't understand this string. Maybe in '-march=$LIBOMPTARGET_AMDGCN_TEST_TARGET'? Seems bad to be hard coding a gfx908 here, presumably it should be whatever is in the system running the tests

openmp/libomptarget/deviceRTLs/CMakeLists.txt
12

would rather enable this unconditionally, which works today except for people who build with the amdgpu target disabled. See D98746

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
94

This we can patch separately. Maybe also drop the gfx7? I don't think it's being tested, and if it's broken it would be friendlier to not build it by default.

openmp/libomptarget/plugins/CMakeLists.txt
80

This seems to be a regression, why not continue building the plugin unconditionally?

openmp/libomptarget/test/lit.site.cfg.in
19

this seems very inconvenient for the tests, I think we should handle missing march by looking at the system we're running the tests on

pdhaliwal added inline comments.Mar 31 2021, 7:34 AM
openmp/libomptarget/CMakeLists.txt
40

Its just a default cmake option value. Since there is no automatic march detection, I thought of having it configurable from command line. I will need dig around for detecting system gpu.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
94

I will create a separate patch for it.

openmp/libomptarget/plugins/CMakeLists.txt
80

This was causing issues when plugin was always getting built but deviceRTL not. When plugin is built, the runtime tests will be executed which are going to fail.

openmp/libomptarget/test/lit.site.cfg.in
19

Agreed. This seems like a problem which could be solved by Driver/ToolChain.

pdhaliwal updated this revision to Diff 336060.Apr 8 2021, 4:44 AM
  • Addressed review comments.

I guess there's no XFAIL equivalent here? In that case we should probably leave off the RUN line for the tests that can't work, as they'll otherwise break the build once an amdgpu CI machine goes live

It's a general problem of the current libomptarget tests. A test fails if any of the run lines fails and will not check the remaining lines.
I think, it would be better to run the tests individually for each available target. Would it be possible to run the test for each supported target in a different work directory?

pdhaliwal updated this revision to Diff 340447.Apr 26 2021, 1:39 AM

Add four more tests.

pdhaliwal updated this revision to Diff 340448.Apr 26 2021, 1:43 AM

Remove whitespace change.

I guess there's no XFAIL equivalent here? In that case we should probably leave off the RUN line for the tests that can't work, as they'll otherwise break the build once an amdgpu CI machine goes live

It's a general problem of the current libomptarget tests. A test fails if any of the run lines fails and will not check the remaining lines.
I think, it would be better to run the tests individually for each available target. Would it be possible to run the test for each supported target in a different work directory?

With properly configured cmake and lit, it should be possible to add tests run separately for architectures. For e.g. it should be possible to create targets like check-libomptarget-amdgcn-amd-amdhsa which would run tests only for amdgcn and similar for others. These can be grouped under existing check-libomptarget.

With properly configured cmake and lit, it should be possible to add tests run separately for architectures. For e.g. it should be possible to create targets like check-libomptarget-amdgcn-amd-amdhsa which would run tests only for amdgcn and similar for others. These can be grouped under existing check-libomptarget.

I posted D101315 and D101326. I suggest that you rebase on these two patches. This should cut down the necessary changes for your patch.

You might want to mark unsupported tests with // XFAIL: amdgcn-amd-amdhsa (currently failing, but will be fix in future) or // UNSUPPORTED: amdgcn-amd-amdhsa (no plan to support the test)

pdhaliwal edited the summary of this revision. (Show Details)Apr 28 2021, 6:57 AM

Patch is much improved by Joachim's refactor!

Looks good to me, will leave the check box to someone who has edited these tests before. Bringing printf online looks like a useful thing to do soon.

openmp/libomptarget/test/unified_shared_memory/close_enter_exit.c
4 ↗(On Diff #341080)

Inconsistent with above. I.e. XFAIL on two lines or one XFAIL with comma delimited. No preference either way here, but probably stay with a convention

It might help in the future, if you add comments for each XFAIL you add, saying what feature is missing to make the test case work (like you did with printf).
-> This would naturally result in a separate XFAIL line for each offloading target.

Add failure reason for the failing tests.

protze.joachim accepted this revision.Apr 30 2021, 6:33 AM

Thanks!
lgtm

This revision is now accepted and ready to land.Apr 30 2021, 6:33 AM
This revision was automatically updated to reflect the committed changes.

Where's the logic to skip running these tests if ./amdgpu-arch fails?

Where's the logic to skip running these tests if ./amdgpu-arch fails?

The tests are included to check-libomptarget, if llvm-project/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt reaches the last line, i.e., if the plugin is built.

Where's the logic to skip running these tests if ./amdgpu-arch fails?

The tests are included to check-libomptarget, if llvm-project/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt reaches the last line, i.e., if the plugin is built.

Ah, that's not ideal. Picked up the same behaviour as nvptx then.

The plugin existing is necessary but not sufficient for running the tests to succeed. One requires the plugin, but also the hardware available locally.

amdgpu-arch returning 0 will correlate strongly with there being a local gpu that tests can run on. Do we have a good place to put a call to that which would control whether to run the amdgpu tests?

We should be able to use a successful call to one of the binaries in the cuda toolkit to do the same test for nvidia, so that people with cuda installed but no nvidia gpu don't run the nvptx tests.