[Openmp-commits] [PATCH] D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions.

Sat Apr 13 06:04:03 PDT 2019

Hahnfeld added a comment.

In D60578#1465420 <https://reviews.llvm.org/D60578#1465420>, @hfinkel wrote:

> In D60578#1465371 <https://reviews.llvm.org/D60578#1465371>, @Hahnfeld wrote:
>
> > I think we need to be careful about adding `nvptx` tests to `libomptarget/test`: They can be executed using Clang later than 6.0.0, but that version wasn't able to offload to GPUs. Given that the changes are limited to `libomptarget-nvptx` (because its parallelism is kind of special), I think the new test should go to `libomptarget/deviceRTLs/nvptx/test`. Just my 2 cents...
>
>
> I don't see anything in this test that is nvptx specific. Is there something about the semantics that make it specific to nvptx? We need to build of a suite of tests for accelerator offloading in general. We'll have other accelerator backends (e.g., for AMD GPUs), and the offloading tests should apply to them too.

First, I agree that the test is not specific to `nvptx` and should pass for all targets. However (at least so far) `libomptarget/test` is for tests that exercise the target-agnostic part of `libomptarget`; like starting `target` regions, environment variables, mapping etc.

> Also, I don't understand what Clang 6 support has to do with adding tests... clearly, we'll add tests for bugs, or already have, that will then fail on older versions of Clang.

In that case these tests need to be marked `UNSUPPORTED` for versions of Clang that will not pass them. There's infrastructure for that, but it's not applied in the current form of this patch.

> And if I target is not supported, the associated libomptarget-compile-run is ignored, no?

Yes and no: Yes, if a plugin (meaning the library that interfaces with the vendor libraries for launching `target` regions) is not compiled, the `libomptarget-compile-run` for that target expands to echos. However, there is currently no way of finding out if a given test can actually run (for example there needs to be a GPU plugged into the system). In theory you could use vendor commands like `nvidia-smi` to query that, but that still does not guarantee that the tests have a chance to pass (because of various reasons; the one that I care most about is that we have our GPUs configured in Exclusive mode, so if there's a process already running on the GPU, all others that try to create a context will get a runtime error) and IMHO it would be a poor development if `check-openmp` would simply stop to work in these cases.
(Side note: CUDA in Clang does the same, they have tests in `test-suite` that can actually be run on real hardware; the Clang tests just check the generated code.)

I can see your point that having generic tests live below `libomptarget-nvptx` is not ideal, but I think it's the best place we have right now given that apparently nobody plans to work on more infrastructure (which is sad).

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60578