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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

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.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60578