[Openmp-commits] [PATCH] D101498: [OpenMP] Test unified shared memory tests only on systems that support it.

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added a comment.

In D101498#2729100 <https://reviews.llvm.org/D101498#2729100>, @protze.joachim wrote:

> In D101498#2727812 <https://reviews.llvm.org/D101498#2727812>, @Meinersbur wrote:
>
>> In D101498#2724916 <https://reviews.llvm.org/D101498#2724916>, @protze.joachim wrote:
>>
>>> The logic to identify whether certain features are supported for testing is in `openmp/cmake/OpenMPTesting.cmake`.
>>
>> The file does not contain target-specific features, but "Set up testing infrastructure" (According to the comment in the top-level CMakeLists.txt). At the point when it is invoked, libomptarget has not been processed yet, making it impossible to determine target-dependent features.

The add_openmp_testsuite call in the dest directory tests at the moment has no differentiation between different targets. To you intend a

  if (CURRENT_TARGET STREQUAL "nvptx64-nvidia-cuda")

in there?
Alternatively, add_openmp_testsuite could be called from the plugins themselves where these feature tests occur anyway (my preferred option).

However, I don't see a gain, we still have to pass a new cmake variable to lit; E.g. UNSUPPORTED suggestion still requires LIBOMPTARGET_DEP_CUDA_ARCH.

> For me it looks like `REQUIRES: unified_shared_memory` might not be sharp enough to mark those tests.
> Probably we should go with `UNSUPPORTED: sm_35, sm_37, sm_50, sm_52, sm_53, sm_60,  sm_61, sm_62` and set the value for the architecture available in the system.

Not disagreeing that an additional `sm_*` items in the feature list would not be useful, but `REQUIRES: unified_shared_memory` is much more descriptive and does not require updating each and every test file if it changes (either because new target without USM are added or patches such as D101595 <https://reviews.llvm.org/D101595>).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101498/new/

https://reviews.llvm.org/D101498