Hi Julian!
On 2020-06-25T13:36:15+0200, I wrote:
> On 2020-06-16T15:39:42-0700, Julian Brown <jul...@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
