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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D60578#1463642 <https://reviews.llvm.org/D60578#1463642>, @jdoerfert wrote:

> In D60578#1463583 <https://reviews.llvm.org/D60578#1463583>, @ABataev wrote:
>
> > In D60578#1463545 <https://reviews.llvm.org/D60578#1463545>, @jdoerfert wrote:
> >
> > > In D60578#1463461 <https://reviews.llvm.org/D60578#1463461>, @ABataev wrote:
> > >
> > > > In D60578#1463452 <https://reviews.llvm.org/D60578#1463452>, @jdoerfert wrote:
> > > >
> > > > > In D60578#1463411 <https://reviews.llvm.org/D60578#1463411>, @gtbercea wrote:
> > > > >
> > > > > > LGTM
> > > > >
> > > > >
> > > > > Is there a way we can test this?
> > > >
> > > >
> > > > It is tested in the internal testsuite, don't know when it is going to be committed to trunk
> > >
> > >
> > > There are two problems:
> > >
> > > 1. The internal testsuite did run before this patch, right? So it is unclear what that means.
> >
> >
> > No, the tests ran with this patch.
>
>
> The internal test suite did run before this commit as well even though it was buggy. It is unclear to me what "the tests" does therefore mean.
>  Now you might have added some tests which nobody can check but we just see some changes that add "+ 1". 
>  How should one review this? Similarly important, how should one now ensure this doesn't break in the future?
>
> >> 2. Changes done upstream might break this without us noticing for a while and without being able to know apriory.
> > 
> > We test everything before doing any changes.
>
> The problem is not that you do not test everything, the problem is that the rest cannot.
>
> >> Why don't we have unit tests here or in the llvm-test suite?
> > 
> > Because this is the library. Do you have an idea how to write the unit tests for it? It can be tested only with the executable tests.
>
> We write google unit tests for various components, maybe something like that works here as well. A test that makes sure the initial output of `omp_get_level` is now 1 would then be great. It is by far not trivial to determine that `omp_get_level`, if called with an uninitialized device RT, should return `parallelLevel + 1`.
>
> > I know, that someone worked on the target-based testsuite, but don't know when it is going to be ready.
>
> There is the V&V test suite: https://crpl.cis.udel.edu/ompvvsollve/
>  We could also add openmp target tests into the LLVM Test Suite and run them if people define CMAKE flags.


Actually, it was the testsuite, which reveals the problems with the runtime. But only after some changes in the compiler I made to run more constructs in SPMD. Before that they all were executed in non-SPMD and the problem was masked. And I don't see a problem here since the exhaustive testing is impossible in principle.
If you have a testsuite and ready to prepare and send an RFC, solve the problems with the license, organize it, setup buildbots, provide support, then go ahead. We can do everything, but it requires a lot of time. I agree that we need target-specific testing.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60578