Page MenuHomePhabricator

[OpenMP][libomptarget][test] Separate lit tests for different offloading targets (2/2)
ClosedPublic

Authored by protze.joachim on Apr 26 2021, 2:27 PM.

Details

Summary

The first part of this patch (D101315) separates libomptarget tests into a lit instance per offloading target.
This allows to fuse the RUN lines for most tests.

This patch updates the RUN lines in libomptarget tests to use a generic run line independent of the offloading target selected for the lit instance.

In cases, where no RUN line was defined for a specific offloading target, the corresponding target is declared as XFAIL. If it turns out that a test actually supports the target, the XFAIL line can be removed.

Diff Detail

Event Timeline

protze.joachim created this revision.Apr 26 2021, 2:27 PM
protze.joachim requested review of this revision.Apr 26 2021, 2:27 PM
JonChesterfield accepted this revision.Apr 26 2021, 4:01 PM

The signal to noise improvement here is superb, thank you.

It looks like the phabricator stack isn't used for the CI process, so it's probably worth landing D101315 and rebasing before landing this one.

This revision is now accepted and ready to land.Apr 26 2021, 4:01 PM

I rerun the tests for x86_64 and nvptx and removed the XFAIL for all tests succeeding in repeated tests.

With this update of the patch, the same 3 tests continue to fail, which also failed before the patch:

********************
Failed Tests (3):
  libomptarget :: x86_64-pc-linux-gnu :: offloading/bug49334.cpp
  libomptarget :: x86_64-pc-linux-gnu :: offloading/memory_manager.cpp
  libomptarget :: x86_64-pc-linux-gnu :: offloading/parallel_offloading_map.cpp
protze.joachim added inline comments.Apr 27 2021, 3:38 AM
openmp/libomptarget/test/offloading/d2d_memcpy.c
1

For this test, only 2 out of 5 RUN lines had -allow-empty. I'm not totally sure, but does the flag make any sense when there is a CHECK line in the tests?

protze.joachim updated this revision to Diff 340785.EditedApr 27 2021, 3:50 AM

Rebased to master, fixing one conflict.

kkwli0 added a subscriber: kkwli0.Apr 29 2021, 11:02 AM

The following regressions seem to be caused by this patch on Power9+V100.

Unexpectedly Passed Tests (4):
  libomptarget :: nvptx64-nvidia-cuda :: unified_shared_memory/api.c
  libomptarget :: nvptx64-nvidia-cuda :: unified_shared_memory/close_enter_exit.c
  libomptarget :: nvptx64-nvidia-cuda :: unified_shared_memory/close_modifier.c
  libomptarget :: nvptx64-nvidia-cuda :: unified_shared_memory/shared_update.c

https://reviews.llvm.org/D101498 will fix this by allowing to distinguish
platforms supporting unified shared memory from platforms that don't.
Do you think, that looking at the gpu generation is sufficient, or does the
host architecture matter as well?

Kelvin Li via Phabricator <reviews@reviews.llvm.org> schrieb am Do., 29.
Apr. 2021, 20:02:

kkwli0 added a comment.

The following regressions seem to be caused by this patch on Power9+V100.

Unexpectedly Passed Tests (4):
  libomptarget :: nvptx64-nvidia-cuda :: unified_shared_memory/api.c
  libomptarget :: nvptx64-nvidia-cuda ::

unified_shared_memory/close_enter_exit.c

libomptarget :: nvptx64-nvidia-cuda ::

unified_shared_memory/close_modifier.c

libomptarget :: nvptx64-nvidia-cuda ::

unified_shared_memory/shared_update.c

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D101326/new/

https://reviews.llvm.org/D101326