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

[Thomas Schwinge]

Hi!

On Tue, 30 Aug 2016 14:55:06 -0700, Cesar Philippidis <ce...@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