[PATCH] Fix component mappings with derived types for OpenACC
Thomas Schwinge
thomas@codesourcery.com
Tue May 19 12:23:34 GMT 2020
Hi!
On 2020-01-28T13:41:00+0000, Julian Brown <julian@codesourcery.com> wrote:
> On Fri, 24 Jan 2020 10:58:49 +0100
> Tobias Burnus <tobias@codesourcery.com> wrote:
>> the gfortran part is rather obvious and it and the test case look
>> fine to me → OK.
>> The oacc-mem.c also looks okay, but I assume Thomas needs to
>> rubber-stamp it.
>
> I understand that Thomas is unavailable for the time being, so won't be
> able to use his rubber-stamp powers. I added the offending libgomp code
> to start with though, so I think I can go ahead and commit the patch.
> I'll hold off for 24 hours though in case there are any objections
> (Jakub?).
So, in the end you didn't commit this, and now we've got the "un-fixed"
OpenACC/Fortran code generation in the GCC 10.1 release, so have to deal
with it in one way or another going forward, regarding libgomp ABI
compatibility. (Ideally), we need to make sure that "un-fixed" GCC
10.1-built executables continue to work "as good as before" when
dynamically linked/running against "fixed" GCC 10.1+ shared libraries.
Do we get such desired behavior by the patch quoted below? In
particular: removing the 'GOMP_MAP_STRUCT' handling code, but leaving in
the empty 'case GOMP_MAP_STRUCT:' as a no-op, so that we don't run into
'default:' case 'goacc_exit_data_internal UNHANDLED kind'? Is that
sufficient?
Is my understanding correct that "fixed" GCC won't generate such
'GOMP_MAP_STRUCT' anymore (I have't studied in detail), and this empty
'case GOMP_MAP_STRUCT:' only remains in here for backwards compatibility?
In this case, please add a comment to the code, stating this. Otherwise,
please add a comment why "do nothing" is appropriate for
'GOMP_MAP_STRUCT'. In particular, for both scenarios, why we don't need
to skip the following 'sizes[i]' mappings?
Grüße
Thomas
>> On 1/10/20 2:49 AM, Julian Brown wrote:
>> > This patch fixes a bug with mapping Fortran components (i.e. with
>> > the manual deep-copy support) which themselves have derived types.
>> > I've also added a couple of new tests to make sure such mappings
>> > are lowered correctly, and to check for the case that Tobias found
>> > in the message:
>> >
>> > https://gcc.gnu.org/ml/gcc-patches/2020-01/msg00215.html
>> >
>> > The previous incorrect mapping was causing the error condition
>> > mentioned in that message to fail to trigger, and I think (my!)
>> > code in libgomp (goacc_exit_data_internal) to handle
>> > GOMP_MAP_STRUCT specially was papering over the bad mapping also.
>> > Oops!
>> >
>> > I haven't attempted to implement the (harder) sub-copy detection, if
>> > that is indeed supposed to be forbidden by the spec. This patch
>> > should get us to the same behaviour in Fortran as in C & C++ though.
>> >
>> > Tested with offloading to nvptx, also with the (uncommitted)
>> > reference-count self-checking patch enabled.
>> >
>> > OK?
>> >
>> > Thanks,
>> >
>> > Julian
>> >
>> > ChangeLog
>> >
>> > gcc/fortran/
>> > * trans-openmp.c (gfc_trans_omp_clauses): Use inner not decl
>> > for components with derived types.
>> >
>> > gcc/testsuite/
>> > * gfortran.dg/goacc/mapping-tests-3.f90: New test.
>> > * gfortran.dg/goacc/mapping-tests-4.f90: New test.
>> >
>> > libgomp/
>> > * oacc-mem.c (goacc_exit_data_internal): Remove special
>> > (no-copyback) behaviour for GOMP_MAP_STRUCT.
>> >
>> > component-mappings-derived-types-1.diff
>> >
>> > commit 5e9d8846bbaa33a9bdb08adcf1ee9f224a8e8fc0
>> > Author: Julian Brown<julian@codesourcery.com>
>> > Date: Wed Jan 8 15:57:46 2020 -0800
>> >
>> > Fix component mappings with derived types for OpenACC
>> >
>> > gcc/fortran/
>> > * trans-openmp.c (gfc_trans_omp_clauses): Use inner
>> > not decl for components with derived types.
>> >
>> > gcc/testsuite/
>> > * gfortran.dg/goacc/mapping-tests-3.f90: New test.
>> > * gfortran.dg/goacc/mapping-tests-4.f90: New test.
>> >
>> > libgomp/
>> > * oacc-mem.c (goacc_exit_data_internal): Remove
>> > special (no-copyback) behaviour for GOMP_MAP_STRUCT.
>> >
>> > diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
>> > index 9835d2aae3c..efc392d7fa6 100644
>> > --- a/gcc/fortran/trans-openmp.c
>> > +++ b/gcc/fortran/trans-openmp.c
>> > @@ -2783,9 +2783,9 @@ gfc_trans_omp_clauses (stmtblock_t *block,
>> > gfc_omp_clauses *clauses, }
>> > else
>> > {
>> > - OMP_CLAUSE_DECL (node) = decl;
>> > + OMP_CLAUSE_DECL (node) = inner;
>> > OMP_CLAUSE_SIZE (node)
>> > - = TYPE_SIZE_UNIT (TREE_TYPE (decl));
>> > + = TYPE_SIZE_UNIT (TREE_TYPE (inner));
>> > }
>> > }
>> > else if (lastcomp->next
>> > diff --git a/gcc/testsuite/gfortran.dg/goacc/mapping-tests-3.f90
>> > b/gcc/testsuite/gfortran.dg/goacc/mapping-tests-3.f90 new file mode
>> > 100644 index 00000000000..312f596461e
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gfortran.dg/goacc/mapping-tests-3.f90
>> > @@ -0,0 +1,15 @@
>> > +! { dg-options "-fopenacc -fdump-tree-omplower" }
>> > +
>> > +subroutine foo
>> > + type one
>> > + integer i, j
>> > + end type
>> > + type two
>> > + type(one) A, B
>> > + end type
>> > +
>> > + type(two) x
>> > +
>> > + !$acc enter data copyin(x%A)
>> > +! { dg-final { scan-tree-dump-times "omp target
>> > oacc_enter_exit_data map\\(struct:x \\\[len: 1\\\]\\) map\\(to:x.a
>> > \\\[len: \[0-9\]+\\\]\\)" 1 "omplower" } } +end diff --git
>> > a/gcc/testsuite/gfortran.dg/goacc/mapping-tests-4.f90
>> > b/gcc/testsuite/gfortran.dg/goacc/mapping-tests-4.f90 new file mode
>> > 100644 index 00000000000..6257af942df --- /dev/null
>> > +++ b/gcc/testsuite/gfortran.dg/goacc/mapping-tests-4.f90
>> > @@ -0,0 +1,17 @@
>> > +subroutine foo
>> > + type one
>> > + integer i, j
>> > + end type
>> > + type two
>> > + type(one) A, B
>> > + end type
>> > +
>> > + type(two) x
>> > +
>> > +! This is accepted at present, although it represents a
>> > probably-unintentional +! overlapping subcopy.
>> > + !$acc enter data copyin(x%A, x%A%i)
>> > +! But this raises an error.
>> > + !$acc enter data copyin(x%A, x%A%i, x%A%i)
>> > +! { dg-error ".x.a.i. appears more than once in map clauses"
>> > "" { target "*-*-*" } 15 } +end
>> > diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c
>> > index 2d4bba78efd..232683a85f0 100644
>> > --- a/libgomp/oacc-mem.c
>> > +++ b/libgomp/oacc-mem.c
>> > @@ -1136,38 +1136,6 @@ goacc_exit_data_internal (struct
>> > gomp_device_descr *acc_dev, size_t mapnum, break;
>> >
>> > case GOMP_MAP_STRUCT:
>> > - {
>> > - int elems = sizes[i];
>> > - for (int j = 1; j <= elems; j++)
>> > - {
>> > - struct splay_tree_key_s k;
>> > - k.host_start = (uintptr_t) hostaddrs[i + j];
>> > - k.host_end = k.host_start + sizes[i + j];
>> > - splay_tree_key str;
>> > - str = splay_tree_lookup (&acc_dev->mem_map, &k);
>> > - if (str)
>> > - {
>> > - if (finalize)
>> > - {
>> > - if (str->refcount != REFCOUNT_INFINITY)
>> > - str->refcount -= str->virtual_refcount;
>> > - str->virtual_refcount = 0;
>> > - }
>> > - if (str->virtual_refcount > 0)
>> > - {
>> > - if (str->refcount != REFCOUNT_INFINITY)
>> > - str->refcount--;
>> > - str->virtual_refcount--;
>> > - }
>> > - else if (str->refcount > 0
>> > - && str->refcount != REFCOUNT_INFINITY)
>> > - str->refcount--;
>> > - if (str->refcount == 0)
>> > - gomp_remove_var_async (acc_dev, str, aq);
>> > - }
>> > - }
>> > - i += elems;
>> > - }
>> > break;
>> >
>> > default:
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
More information about the Gcc-patches
mailing list