diff mbox series

Fix component mappings with derived types for OpenACC

Message ID 20200110014945.5643ace5@squid.athome
State New
Headers show
Series Fix component mappings with derived types for OpenACC | expand

Commit Message

Julian Brown Jan. 10, 2020, 1:49 a.m. UTC 
Hi,

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.

Comments

Tobias Burnus Jan. 24, 2020, 9:58 a.m. UTC | #1 
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.

Tobias

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:
Julian Brown Jan. 28, 2020, 1:41 p.m. UTC | #2 
On Fri, 24 Jan 2020 10:58:49 +0100
Tobias Burnus <tobias@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<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:
Thomas Schwinge May 19, 2020, 12:23 p.m. UTC | #3 
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
Julian Brown June 4, 2020, 1:58 p.m. UTC | #4 
On Tue, 19 May 2020 14:23:34 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

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

I'm not sure if I got the threading right, but I've now followed up on
this discussion here:

  https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547300.html

Thanks,

Julian
diff mbox series

Patch

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: