On Fri, 24 Jan 2020 10:58:49 +0100 Tobias Burnus <tob...@codesourcery.com> wrote:
> Hi Julian, > > 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?). Thanks, Julian > 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<jul...@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:
