Re: [PATCH] [og12] OpenACC: Don't gang-privatize artificial variables

[Thomas Schwinge <thomas@codesourcery.com>]

Hi Julian!

On 2022-10-14T13:38:56+0000, Julian Brown <julian@codesourcery.com> wrote:
> This patch prevents compiler-generated artificial variables from being
> treated as privatization candidates for OpenACC.
>
> The rationale is that e.g. "gang-private" variables actually must be
> shared by each worker and vector spawned within a particular gang, but
> that sharing is not necessary for any compiler-generated variable (at
> least at present, but no such need is anticipated either).  Variables on
> the stack (and machine registers) are already private per-"thread"
> (gang, worker and/or vector), and that's fine for artificial variables.

OK, that seems fine rationale for this change in behavior.
No contradicting test case jumped onto me, either.

> Several tests need their scan output patterns adjusted to compensate.

ACK -- surprisingly few.  (Some minor fine-tuning necessary for GCC
master branch, as had to be expected; I'm working on that.)

> --- a/gcc/omp-low.cc
> +++ b/gcc/omp-low.cc
> @@ -11400,6 +11400,28 @@ oacc_privatization_candidate_p (const location_t loc, const tree c,
>       }
>      }
>
> +  /* If an artificial variable has been added to a bind, e.g.
> +     a compiler-generated temporary structure used by the Fortran front-end, do
> +     not consider it as a privatization candidate.  Note that variables on
> +     the stack are private per-thread by default: making them "gang-private"
> +     for OpenACC actually means to share a single instance of a variable
> +     amongst all workers and threads spawned within each gang.
> +     At present, no compiler-generated artificial variables require such
> +     sharing semantics, so this is safe.  */
> +
> +  if (res && DECL_ARTIFICIAL (decl))
> +    {
> +      res = false;
> +
> +      if (dump_enabled_p ())
> +     {
> +       oacc_privatization_begin_diagnose_var (l_dump_flags, loc, c, decl);
> +       dump_printf (l_dump_flags,
> +                    "isn%'t candidate for adjusting OpenACC privatization "
> +                    "level: %s\n", "artificial");
> +     }
> +    }

In the source code comment, you say "added to a bind", and that's indeed
what I was expecting, too, and thus put in:

       if (res && DECL_ARTIFICIAL (decl))
         {
    +      gcc_checking_assert (block);
    +
           res = false;

..., but to my surprised, that did fire in one occasion:

> --- a/libgomp/testsuite/libgomp.oacc-fortran/privatized-ref-2.f90
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/privatized-ref-2.f90
> @@ -94,9 +94,7 @@ contains
>      !$acc parallel copy(array)
>      !$acc loop gang private(array) ! { dg-line l_loop[incr c_loop] }
>      ! { dg-note {variable 'i' in 'private' clause isn't candidate for adjusting OpenACC privatization level: not addressable} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'array\.[0-9]+' in 'private' clause is candidate for adjusting OpenACC privatization level} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'array\.[0-9]+' ought to be adjusted for OpenACC privatization level: 'gang'} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'array\.[0-9]+' adjusted for OpenACC privatization level: 'gang'} "" { target { ! { openacc_host_selected || { openacc_nvidia_accel_selected && __OPTIMIZE__ } } } } l_loop$c_loop }
> +    ! { dg-note {variable 'array\.[0-9]+' in 'private' clause isn't candidate for adjusting OpenACC privatization level: artificial} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-message {sorry, unimplemented: target cannot support alloca} PR65181 { target openacc_nvidia_accel_selected } l_loop$c_loop }
>      do i = 1, 10
>        array(i) = 9*i

... here.  Note "variable 'array\.[0-9]+' in 'private' clause";
everywhere else we have "declared in block".

As part of your verification, have you already looked into whether the
new behavior is correct here, or does this one need to continue to be
"adjusted for OpenACC privatization level: 'gang'"?  If the latter,
should we check 'if (res && block && DECL_ARTIFICIAL (decl))' instead of
'if (res && DECL_ARTIFICIAL (decl))', or is there some wrong setting of
'DECL_ARTIFICIAL' -- or are we maybe looking at an inappropriate 'decl'?
(Thinking of commit r12-7580-g7a5e036b61aa088e6b8564bc9383d37dfbb4801e
"[OpenACC privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774]",
for example.)


Grüße
 Thomas


> @@ -122,9 +120,7 @@ contains
>      ! { dg-note {variable 'str' in 'private' clause is candidate for adjusting OpenACC privatization level} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-note {variable 'str' ought to be adjusted for OpenACC privatization level: 'gang'} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-note {variable 'str' adjusted for OpenACC privatization level: 'gang'} "" { target { ! { openacc_host_selected || { openacc_nvidia_accel_selected && __OPTIMIZE__ } } } } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' declared in block is candidate for adjusting OpenACC privatization level} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' ought to be adjusted for OpenACC privatization level: 'gang'} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' adjusted for OpenACC privatization level: 'gang'} "" { target { ! { openacc_host_selected || { openacc_nvidia_accel_selected && __OPTIMIZE__ } } } } l_loop$c_loop }
> +    ! { dg-note {variable 'char\.[0-9]+' declared in block isn't candidate for adjusting OpenACC privatization level: artificial} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-message {sorry, unimplemented: target cannot support alloca} PR65181 { target openacc_nvidia_accel_selected } l_loop$c_loop }
>      do i = 1, 10
>        str(i:i) = achar(ichar('A') + i)
> @@ -167,9 +163,7 @@ contains
>      ! { dg-note {variable 'scalar' in 'private' clause is candidate for adjusting OpenACC privatization level} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-note {variable 'scalar' ought to be adjusted for OpenACC privatization level: 'gang'} "" { target *-*-* } l_loop$c_loop }
>      ! { dg-note {variable 'scalar' adjusted for OpenACC privatization level: 'gang'} "" { target { ! { openacc_host_selected || { openacc_nvidia_accel_selected && __OPTIMIZE__ } } } } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' declared in block is candidate for adjusting OpenACC privatization level} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' ought to be adjusted for OpenACC privatization level: 'gang'} "" { target *-*-* } l_loop$c_loop }
> -    ! { dg-note {variable 'char\.[0-9]+' adjusted for OpenACC privatization level: 'gang'} "" { target { ! { openacc_host_selected || { openacc_nvidia_accel_selected && __OPTIMIZE__ } } } } l_loop$c_loop }
> +    ! { dg-note {variable 'char\.[0-9]+' declared in block isn't candidate for adjusting OpenACC privatization level: artificial} "" { target *-*-* } l_loop$c_loop }
>      do i = 1, 15
>        scalar(i:i) = achar(ichar('A') + i)
>      end do
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955