Re: [PATCH 6/9] [OpenACC] Set bias to zero for explicit attach/detach clauses in C and C++

[Thomas Schwinge <thomas@codesourcery.com>]

Hi Julian!

On 2020-06-25T13:36:15+0200, I wrote:
> On 2020-06-16T15:39:42-0700, Julian Brown <julian@codesourcery.com> wrote:
>> This is a fix for the pointer (or array) size inadvertently being used
>> for the bias of attach and detach clauses (PR95270)
>
> Thanks for looking into that one, which had caused my some gray hair.
>
>> for C and C++.
>
> That means, there is no such problem for Fortran?  (I haven't run into
> one, just curious.)

Looking into something else, I've now found the very same (?) problem for
Fortran, too.  :-| For the following simple testcase, I again do see
non-zero 'bias: 64' for 'enter data attach(data_p)':

    program main
      use openacc
      implicit none
      !TODO Per PR96080, data types chosen so that we can create a "pointer object 'data_p'" on the device.
      integer, dimension(:), target :: data(1)
      integer, dimension(:), pointer :: data_p

      !TODO Per PR96080, not using OpenACC/Fortran runtime library routines.

      !$acc enter data create(data)
      data_p => data
      !$acc enter data copyin(data_p)

      !$acc enter data attach(data_p)
    end program main

..., and the 'attach' fails with 'libgomp: pointer target not mapped for
attach'.  It doesn't fail when I force 'bias = 0' in
'gomp_attach_pointer'.

I've tried a bit, but it seems a bit difficult in Fortran to verify (with
'associated(data_p, data)' etc.) what we've actually 'attach'ed: per
PR96080, a 'call acc_update_self(data_p)' may not be doing the expected
thing, and a '!$acc update self(data_p)' per
'libgomp/oacc-parallel.c:GOACC_update' will update the actual data, but
is no-op for 'GOMP_MAP_TO_PSET', 'GOMP_MAP_POINTER'.  I've stopped
experimenting with that any further.

So it seems Fortran front end changes will also be required in addition
to the C, C++ front end changes you've come up with.  (For avoidance of
doubt: OK to do separately, if you'd like to.  Please also reference GCC
PR95270 for these, and include the testcase from above, or something
better.)


Grüße
 Thomas


> In principle, yes, for master and releases/gcc-10 branches, but please
> incorporate the following items:
>
>>      PR middle-end/95270
>>
>>      gcc/c/
>>      * c-typeck.c (c_finish_omp_clauses): Set OMP_CLAUSE_SIZE (bias) to zero
>>      for standalone attach/detach clauses.
>>
>>      gcc/cp/
>>      * semantics.c (finish_omp_clauses): Likewise.
>>
>>      gcc/testsuite/
>>      * c-c++-common/goacc/mdc-1.c: Update expected dump output for zero
>>      bias.
>> ---
>>  gcc/c/c-typeck.c                         |  8 ++++++++
>>  gcc/cp/semantics.c                       |  8 ++++++++
>>  gcc/testsuite/c-c++-common/goacc/mdc-1.c | 14 +++++++-------
>>  3 files changed, 23 insertions(+), 7 deletions(-)
>
>> --- a/gcc/c/c-typeck.c
>> +++ b/gcc/c/c-typeck.c
>> @@ -14533,6 +14533,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>              }
>>            if (c_oacc_check_attachments (c))
>>              remove = true;
>> +          if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>> +              && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
>> +                  || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
>> +            OMP_CLAUSE_SIZE (c) = size_zero_node;
>>            break;
>>          }
>>        if (t == error_mark_node)
>> @@ -14546,6 +14550,10 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>            remove = true;
>>            break;
>>          }
>> +      if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>> +          && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
>> +              || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
>> +        OMP_CLAUSE_SIZE (c) = size_zero_node;
>>        if (TREE_CODE (t) == COMPONENT_REF
>>            && OMP_CLAUSE_CODE (c) != OMP_CLAUSE__CACHE_)
>>          {
>
> I cannot comment if these two code paths are good places (and the only
> ones) that need to set 'OMP_CLAUSE_SIZE', so I'll trust you've found the
> best/all places.
>
> Does that override an 'OMP_CLAUSE_SIZE' that was set earlier, or
> initialize it?  Maybe the latter, given my comment in
> <https://gcc.gnu.org/PR95270>: "make sure to skip/invalidate the
> 'gcc/gimplify.c:gimplify_scan_omp_clauses' handling"?
>
> Plase add some commentary here in the code, instead of just in the
> ChangeLog, something like: "initialize here, so that gimplify doesn't
> wrongly do so later" (if that's what it is, and in proper language, of
> course).
>
>> --- a/gcc/cp/semantics.c
>> +++ b/gcc/cp/semantics.c
>> @@ -7334,6 +7334,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>              }
>>            if (cp_oacc_check_attachments (c))
>>              remove = true;
>> +          if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>> +              && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
>> +                  || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
>> +            OMP_CLAUSE_SIZE (c) = size_zero_node;
>>            break;
>>          }
>>        if (t == error_mark_node)
>> @@ -7347,6 +7351,10 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>            remove = true;
>>            break;
>>          }
>> +      if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>> +          && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
>> +              || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
>> +        OMP_CLAUSE_SIZE (c) = size_zero_node;
>>        if (REFERENCE_REF_P (t)
>>            && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF)
>>          {
>
> Likewise.
>
>> --- a/gcc/testsuite/c-c++-common/goacc/mdc-1.c
>> +++ b/gcc/testsuite/c-c++-common/goacc/mdc-1.c
>
> Obvious.
>
> In <https://gcc.gnu.org/PR95270> I also requested size vs. bias be
> documented in 'include/gomp-constants.h:enum gomp_map_kind'.
>
> Generally, I'm still somewhat confused by the 'bias' usage in libgomp.
> Is it really only used for the *initial* attach, but then (correctly so?)
> ignored for any later ones?  Please add some commentary next to the
> respective libgomp code.
>
> Please also include an execution test case, like I had included with
> <https://gcc.gnu.org/PR95270>, for example the two files I'm attaching.
> Ah actually, since the directive variant now no longer fails, please
> merge these into one file, with 'test(bool directive)', and two
> 'test(false)', 'test(true)' calls from 'main'.
>
>
> Grüße
>  Thomas


> [ pr95270_-d.c: text/x-csrc ]
> #define DIRECTIVE
> #include "pr95270_-r.c"

> [ pr95270_-r.c: text/x-csrc ]
> // <https://gcc.gnu.org/PR95270>
>
> #include <assert.h>
> #include <openacc.h>
>
> int main()
> {
>   int data;
>   int *data_p_dev = (int *) acc_create(&data, sizeof data);
>   int *data_p = &data;
>   acc_copyin(&data_p, sizeof data_p);
>
> #ifdef DIRECTIVE
> # pragma acc enter data attach(data_p)
> #else
>   {
>     void **ptr = (void **) &data_p;
>     acc_attach(ptr);
>   }
> #endif
>
>   acc_update_self(&data_p, sizeof data_p);
>   assert (data_p == data_p_dev);
>
>   return 0;
> }
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter