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

Sat Apr 13 05:34:49 PDT 2019

hfinkel added a comment.

In D60578#1465371 <https://reviews.llvm.org/D60578#1465371>, @Hahnfeld wrote:

> In D60578#1464350 <https://reviews.llvm.org/D60578#1464350>, @ABataev wrote:
>
> > In D60578#1464304 <https://reviews.llvm.org/D60578#1464304>, @hfinkel wrote:
> >
> > > Our general policy is that all commits that can have tests, should have tests. We have OpenMP target tests in libomptarget/test -- and given that you've added tests there yourself, I assume that you know this ;) -- plus tests in libomptarget/deviceRTLs/nvptx/test - although it sounds like this situation can be triggered using portable code, so I'd prefer we add a test in libomptarget/test. Can you please do that?
> >
> >
> > Sure, if we have a testing infrastructure for this, I'll add the test. Just missed the tests for NVPTX, will definitely add it.
>
>
> 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.  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. And if I target is not supported, the associated libomptarget-compile-run is ignored, no?

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60578