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
Related show

Commit Message

Julian Brown Jan. 10, 2020, 1:49 a.m.
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. | #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. | #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:

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: