[gomp4] fix an ICE involving assumed-size arrays

[gomp4] fix an ICE involving assumed-size arrays

[Cesar Philippidis]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

Usually a data clause would would have OMP_CLAUSE_SIZE set, but not all
do. In the latter case, lower_omp_target falls back to using size of the
type of the variable specified in the data clause. However, in the case
of assumed-size arrays, the size of the type may be NULL because its
undefined. My fix for this solution is to set the size to one byte if
the size of the type is NULL. This solution at least allows the runtime
the opportunity to remap any data already present on the accelerator.
However, if the data isn't present on the accelerator, this will likely
result in some sort of segmentation fault on the accelerator.

The OpenACC spec is not clear how the compiler should handle
assumed-sized arrays when the user does not provide an explicit data
clause with a proper subarray. It was tempting to make such implicit
variables errors, but arguably that would affect usability. Perhaps I
should a warning for implicitly used assumed-sizes arrays?

I've applied this patch to gomp-4_0-branch. It looks like OpenMP has a
similar problem.

Cesar

[-- Attachment #2: gomp4-assumed.diff --]
[-- Type: text/x-patch, Size: 1842 bytes --]

2016-08-30  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* omp-low.c (lower_omp_target): Handle NULL-sized types for
	assumed-sized arrays.

	libgomp/
	* testsuite/libgomp.oacc-fortran/assumed-size.f90: New test.


diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index b314523..0faf6c3 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -16534,6 +16534,12 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
 	      s = OMP_CLAUSE_SIZE (c);
 	    if (s == NULL_TREE)
 	      s = TYPE_SIZE_UNIT (TREE_TYPE (ovar));
+	    /* Fortran assumed-size arrays have zero size because the
+	       type is incomplete.  Set the size to one to allow the
+	       runtime to remap any existing data that is already
+	       present on the accelerator.  */
+	    if (s == NULL_TREE)
+	      s = integer_one_node;
 	    s = fold_convert (size_type_node, s);
 	    purpose = size_int (map_idx++);
 	    CONSTRUCTOR_APPEND_ELT (vsize, purpose, s);
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/assumed-size.f90 b/libgomp/testsuite/libgomp.oacc-fortran/assumed-size.f90
new file mode 100644
index 0000000..79de675
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-fortran/assumed-size.f90
@@ -0,0 +1,31 @@
+! Test if implicitly determined data clauses work with an
+! assumed-sized array variable.  Note that the array variable, 'a',
+! has been explicitly copied in and out via acc enter data and acc
+! exit data, respectively.
+
+program test
+  implicit none
+
+  integer, parameter :: n = 100
+  integer a(n), i
+
+  call dtest (a, n)
+
+  do i = 1, n
+     if (a(i) /= i) call abort
+  end do
+end program test
+
+subroutine dtest (a, n)
+  integer i, n
+  integer a(*)
+
+  !$acc enter data copyin(a(1:n))
+
+  !$acc parallel loop
+  do i = 1, n
+     a(i) = i
+  end do
+
+  !$acc exit data copyout(a(1:n))
+end subroutine dtest

Re: [gomp4] fix an ICE involving assumed-size arrays

[Thomas Schwinge]

Hi!

On Tue, 30 Aug 2016 14:55:06 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> Usually a data clause would would have OMP_CLAUSE_SIZE set, but not all
> do. In the latter case, lower_omp_target falls back to using size of the
> type of the variable specified in the data clause. However, in the case
> of assumed-size arrays, the size of the type may be NULL because its
> undefined. My fix for this solution is to set the size to one byte if
> the size of the type is NULL. This solution at least allows the runtime
> the opportunity to remap any data already present on the accelerator.
> However, if the data isn't present on the accelerator, this will likely
> result in some sort of segmentation fault on the accelerator.
> 
> The OpenACC spec is not clear how the compiler should handle
> assumed-sized arrays when the user does not provide an explicit data
> clause with a proper subarray. It was tempting to make such implicit
> variables errors, but arguably that would affect usability. Perhaps I
> should a warning for implicitly used assumed-sizes arrays?

(I don't know a lot about Fortran assumed-size arrays, but I agree that a
user might expect code to work, like that in the example you added.)

> I've applied this patch to gomp-4_0-branch. It looks like OpenMP has a
> similar problem.

... which Jakub for <https://gcc.gnu.org/PR78866> fixed in trunk r243860,
<http://mid.mail-archive.com/20161221161950.GY21933@tucnak> by
"disallow[ing] explicit or implicit OpenMP mapping of assumed-size
arrays".  So when merging these two changes, I had to apply the following
additional patch, which will need to get resolved some way or another:

--- gcc/fortran/trans-openmp.c
+++ gcc/fortran/trans-openmp.c
@@ -1048,6 +1048,11 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p)
 
   tree decl = OMP_CLAUSE_DECL (c);
 
+  /* This conflicts with the OpenACC changes done to support assumed-size
+     arrays that are implicitly mapped after enter data directive (see
+     libgomp.oacc-fortran/assumed-size.f90) -- doesn't the same apply to
+     OpenMP, too?  */
+#if 0
   /* Assumed-size arrays can't be mapped implicitly, they have to be
      mapped explicitly using array sections.  */
   if (TREE_CODE (decl) == PARM_DECL
@@ -1061,6 +1066,7 @@ gfc_omp_finish_clause (tree c, gimple_seq *pre_p)
 		"implicit mapping of assumed size array %qD", decl);
       return;
     }
+#endif
 
   tree c2 = NULL_TREE, c3 = NULL_TREE, c4 = NULL_TREE;
   if (POINTER_TYPE_P (TREE_TYPE (decl)))
--- gcc/testsuite/gfortran.dg/gomp/pr78866-2.f90
+++ gcc/testsuite/gfortran.dg/gomp/pr78866-2.f90
@@ -3,7 +3,8 @@
 
 subroutine pr78866(x)
   integer :: x(*)
-!$omp target		! { dg-error "implicit mapping of assumed size array" }
+! Regarding the XFAIL, see gcc/fortran/trans-openmp.c:gfc_omp_finish_clause.
+!$omp target		! { dg-error "implicit mapping of assumed size array" "" { xfail *-*-* } }
   x(1) = 1
 !$omp end target
 end

For reference, here are Cesar's gomp-4_0-branch r239874 changes:

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -16534,6 +16534,12 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
>  	      s = OMP_CLAUSE_SIZE (c);
>  	    if (s == NULL_TREE)
>  	      s = TYPE_SIZE_UNIT (TREE_TYPE (ovar));
> +	    /* Fortran assumed-size arrays have zero size because the
> +	       type is incomplete.  Set the size to one to allow the
> +	       runtime to remap any existing data that is already
> +	       present on the accelerator.  */
> +	    if (s == NULL_TREE)
> +	      s = integer_one_node;
>  	    s = fold_convert (size_type_node, s);
>  	    purpose = size_int (map_idx++);
>  	    CONSTRUCTOR_APPEND_ELT (vsize, purpose, s);
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/assumed-size.f90
> @@ -0,0 +1,31 @@
> +! Test if implicitly determined data clauses work with an
> +! assumed-sized array variable.  Note that the array variable, 'a',
> +! has been explicitly copied in and out via acc enter data and acc
> +! exit data, respectively.

(Should add a "dg-do run" directive here?)

> +
> +program test
> +  implicit none
> +
> +  integer, parameter :: n = 100
> +  integer a(n), i
> +
> +  call dtest (a, n)
> +
> +  do i = 1, n
> +     if (a(i) /= i) call abort
> +  end do
> +end program test
> +
> +subroutine dtest (a, n)
> +  integer i, n
> +  integer a(*)
> +
> +  !$acc enter data copyin(a(1:n))
> +
> +  !$acc parallel loop
> +  do i = 1, n
> +     a(i) = i
> +  end do
> +
> +  !$acc exit data copyout(a(1:n))
> +end subroutine dtest


Grüße
 Thomas