Hi!

On 2022-10-18T16:46:07+0200, Thomas Schwinge <thomas@codesourcery.com> wrote:
> 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.)

With those changes...

>> --- 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))'

..., and that change merged in, I've then pushed to master branch
commit 11e811d8e2f63667f60f73731bb934273f5882b8
"OpenACC: Don't gang-privatize artificial variables [PR90115]", see
attached.

Cherry-picked pushed to releases/gcc-12 branch in
commit 9b116c51a451995f1bae8fdac0748fcf3f06aafe
"OpenACC: Don't gang-privatize artificial variables [PR90115]", see
attached.

I've then also done a merge from releases/gcc-12 branch into
devel/omp/gcc-12 branch, taking care of merge conflicts due to
"fine-tuning necessary for GCC master branch", which shouldn't propagate
into devel/omp/gcc-12 branch.  Pushed to devel/omp/gcc-12 branch
commit 33eae55cd9effd9e0bb0c3659cc5dfc100b6fd4e
"Merge commit '9b116c51a451995f1bae8fdac0748fcf3f06aafe'".


Grüße
 Thomas


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