Re: [PATCH 7/7] openmp: Add testcases for metadirectives

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Dec 10, 2021 at 05:40:36PM +0000, Kwok Cheung Yeung wrote:
> This adds testcases for metadirectives.

Let me start with the last patch.

> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-1.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +
> +#define N 100
> +
> +void f (int a[], int b[], int c[])
> +{
> +  #pragma omp metadirective \
> +      default (teams loop) \
> +      default (parallel loop) /* { dg-error "there can only be one default clause in a metadirective before '\\(' token" } */

I'd prefer consistency, check_no_duplicate_clause prints for similar bugs
too many %qs clauses
so it would be nice if this emitted the same (and the before '\\(' token
part would be nice to avoid as well (i.e. use error rather than parse
error).

> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-2.c
> @@ -0,0 +1,74 @@
> +/* { dg-do compile } */
> +
> +#define N 100
> +
> +int main (void)
> +{
> +  int x = 0;
> +  int y = 0;
> +
> +  /* Test implicit default (nothing).  */
> +  #pragma omp metadirective \
> +      when (device={arch("nvptx")}: barrier)
> +    x = 1;

I'm not really sure if device={arch("nvptx")} in main is
the best idea for most of such tests.
Because we should be able to decide that right away, main isn't
declare target to (and better shouldn't be) and so when we know host
isn't nvptx and that it won't be offloaded, it is clear it can't be
that arch.
Of course, we need to test such a case too in a few spots, but it would
be nice to have more diversity in the tests.
One possibility is non-main function with declare target to after the
function definition (but one can't then use teams in the metadirectives).
But would be nice to use more variety in selectors, user, implementation, device
with isa or kind, etc. instead of using always the same thing in most of the
tests.

Also it would be nice to cover say a directive which needs loop, a directive
which needs a normal body and say 2 directives which are standalone.

> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-3.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fdump-tree-original" } */
> +/* { dg-additional-options "-fdump-tree-gimple" } */
> +/* { dg-additional-options "-fdump-tree-optimized" } */
> +
> +#define N 100
> +
> +void f (int x[], int y[], int z[])
> +{
> +  int i;
> +
> +  #pragma omp target map(to: x, y) map(from: z)
> +    #pragma omp metadirective \
> +	when (device={arch("nvptx")}: teams loop) \
> +	default (parallel loop)

It would be nice to have many of the tests where all the metadirective
variants are actually possible.  Here the nvptx variant
is quite unlikely, nvptx is rarely tested as host arch,
f is not declare target to and even if it was, teams is not allowed
inside of target regions like that.

> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-4.c
> +
> +  /* TODO: This does not execute a version of f with the default clause
> +     active as might be expected.  */

Might be nice to mention that it is correct 5.0 behavior, 5.1 is just
broken in this regard and 5.2 changed the behavior so that parallel loop
is actually invoked.

> +  f (a, 2.71828);

> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-5.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fdump-tree-original" } */
> +
> +#define N 100
> +
> +void f (int a[], int flag)
> +{
> +  int i;
> +  #pragma omp metadirective \
> +	when (user={condition(flag)}: \
> +		target teams distribute parallel for map(from: a[0:N])) \
> +	default (parallel for)
> +  for (i = 0; i < N; i++)
> +    a[i] = i;
> +}
> +
> +/* The metadirective should be resolved at parse time.  */

???  How can it?  The above is invalid in OpenMP 5.0 (condition
should be constant expression), it is valid in OpenMP 5.1, but is then
resolved at runtime, certainly not at parse time.

Would be nice to also test user={condition(1)} etc. where it would
be resolved at parse time.
And, please add some tests even with user scores.

> +++ b/gcc/testsuite/c-c++-common/gomp/metadirective-6.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-fdump-tree-original" } */
> +/* { dg-additional-options "-fdump-tree-gimple" } */
> +
> +#define N 100
> +
> +void bar (int a[], int run_parallel, int run_guided)
> +{
> +  #pragma omp metadirective \
> +	when (user={condition(run_parallel)}: parallel)
> +  {
> +    int i;
> +    #pragma omp metadirective \
> +	when (construct={parallel}, user={condition(run_guided)}: \
> +	      for schedule(guided)) \
> +	when (construct={parallel}: for schedule(static))
> +      for (i = 0; i < N; i++)
> +	a[i] = i;
> +   }
> + }
> +
> +/* The outer metadirective should be resolved at parse time.  */
> +/* The inner metadirective should be resolved during Gimplificiation.  */

Again, dynamic condition, so I don't see how this holds, both should be
resolved at runtime.

> +++ b/libgomp/testsuite/libgomp.c-c++-common/metadirective-1.c
> @@ -0,0 +1,35 @@
> +/* { dg-do run } */
> +
> +#define N 100
> +
> +void f (int x[], int y[], int z[])
> +{
> +  int i;
> +
> +  #pragma omp target map(to: x[0:N], y[0:N]) map(from: z[0:N])
> +    #pragma omp metadirective \
> +	when (device={arch("nvptx")}: teams loop) \
> +	default (parallel loop)
> +      for (i = 0; i < N; i++)

This doesn't really test which of them was selected and one of them is
extremely unlikely.  Doesn't hurt to have some such tests, but would be
nice if there were tests that it could vary (e.g. same test triggering
both or all variants in different code paths, with selector chosen
such that it is possible) where it would return different results
and at runtime it would be possible to decide which one is which.
That can be done either through declare variant in the body, or say
nested metadirective which will say through data sharing or mapping
result in different values (say task shared(var) in one case and
task firstprivate(var) in another etc.).

	Jakub