Update to top.

LG

Fixed test checks.

Fixed the comments for the expected results.

In D60918#1473401, @Hahnfeld wrote:

Why is it enough to have one counter per warp, what happens if threads within a warp diverge? Before D55773 we had a counter per thread...

Updated the test after the fix + outlined warp/lane id operations into functions.

LG

Other tests in presence of the requires directive with the clause?

The patch itself looks good, can't say anything about the numbers.

Also, it would be good to try to add the tests for this new functionality

Seems good with a nit

So, the test is good or I should put it into some other directory?

LG

Moved the test to nvptx directory.

Added a test.

In D57059#1463833, @lebedev.ri wrote:

Passing-by question: will this *only* consider non-power-of-two vectors, or all vectors that are smaller than the vector length?
I.e. is this something that will help with e.g. https://godbolt.org/z/h64TuT ? (from IRC)

In D60578#1464304, @hfinkel wrote:

...

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.

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?

In D60578#1463642, @jdoerfert wrote:

In D60578#1463583, @ABataev wrote:

In D60578#1463545, @jdoerfert wrote:

In D60578#1463461, @ABataev wrote:

In D60578#1463452, @jdoerfert wrote:

In 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.

In D60578#1463545, @jdoerfert wrote:

In D60578#1463461, @ABataev wrote:

In D60578#1463452, @jdoerfert wrote:

In 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.

In D60578#1463452, @jdoerfert wrote:

In D60578#1463411, @gtbercea wrote:

LGTM

Is there a way we can test this?