[gomp4] add support for allocatable scalars in OpenACC declare constructs

[Thomas Schwinge]

Hi!

On Wed, 19 Apr 2017 11:11:39 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> I've applied this patch to gomp-4_0-branch to add support for fortran
> allocatable scalars inside OpenACC declare constructs.

Thanks!


> Included in this patch is a bug fix for non-declared allocatable
> scalars. [...]

Please, bug fixes as work items/patches/commits separate from new
features.  (As long as that makes sense.)


I have not reviewed your changes in detail; just a few comments here:

> --- a/gcc/fortran/trans-openmp.c
> +++ b/gcc/fortran/trans-openmp.c
> @@ -3336,6 +3336,18 @@ gfc_trans_oacc_executable_directive (gfc_code *code)
>    gfc_start_block (&block);
>    oacc_clauses = gfc_trans_omp_clauses (&block, code->ext.omp_clauses,
>  					code->loc);
> +
> +  /* Promote GOMP_MAP_FIRSTPRIVATE_POINTER to GOMP_MAP_ALWAYS_POINTER for
> +     variables inside OpenACC update directives.  */

Well, that's quite obvious from the following few lines of code ;-) --
would be more helpful in such cases to describe in source code comments
*why* something is being done.

> +  if (code->op == EXEC_OACC_UPDATE)
> +    for (tree c = oacc_clauses; c; c = OMP_CLAUSE_CHAIN (c))
> +      {
> +        if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> +	    && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER)
> +	  OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER);
> +      }

> --- a/gcc/testsuite/gfortran.dg/goacc/declare-allocatable-1.f90
> +++ b/gcc/testsuite/gfortran.dg/goacc/declare-allocatable-1.f90
> @@ -6,20 +6,20 @@
>  
>  program allocate
>    implicit none
> -  integer, allocatable :: a(:)
> +  integer, allocatable :: a(:), b
>    integer, parameter :: n = 100
>    integer i
> -  !$acc declare create(a)
> +  !$acc declare create(a,b)
>  
> -  allocate (a(n))
> +  allocate (a(n), b)
>  
> -  !$acc parallel loop copyout(a)
> +  !$acc parallel loop copyout(a, b)
>    do i = 1, n
> -     a(i) = i
> +     a(i) = b
>    end do

That "a(i) = b" assignment is reading uninitialized data ("copyout(b)").
Did you mean to specify "copyin(b)" or (implicit?) "firstprivate(b)", and
set "b" to a defined value before the region?

>  
> -  deallocate (a)
> +  deallocate (a, b)
>  end program allocate
>  
> -! { dg-final { scan-tree-dump-times "pragma acc enter data map.declare_allocate" 1 "original" } }
> -! { dg-final { scan-tree-dump-times "pragma acc exit data map.declare_deallocate" 1 "original" } }
> +! { dg-final { scan-tree-dump-times "pragma acc enter data map.declare_allocate" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "pragma acc exit data map.declare_deallocate" 2 "original" } }

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90
> @@ -0,0 +1,30 @@

Missing "{ dg-do run }" to exercise torture testing -- or is that
intentionally not done here?

> +program main
> +  implicit none
> +  integer, parameter :: n = 100
> +  integer, allocatable :: a, c
> +  integer :: i, b(n)
> +
> +  allocate (a)
> +
> +  a = 50
> +
> +  !$acc parallel loop
> +  do i = 1, n;
> +     b(i) = a
> +  end do
> +
> +  do i = 1, n
> +     if (b(i) /= a) call abort
> +  end do
> +
> +  allocate (c)
> +
> +  print *, loc (c)
> +  !$acc parallel copyout(c) num_gangs(1)
> +  c = a
> +  !$acc end parallel
> +
> +  if (c /= a) call abort
> +
> +  deallocate (a, c)
> +end program main


Additionally, I'm seeing one regression:

    [-PASS:-]{+FAIL: gfortran.dg/goacc/pr77371-1.f90   -O  (internal compiler error)+}
    {+FAIL:+} gfortran.dg/goacc/pr77371-1.f90   -O  (test for excess errors)

    [...]/gcc/testsuite/gfortran.dg/goacc/pr77371-1.f90:5:0: internal compiler error: in force_constant_size, at gimplify.c:664
    0x87c57b force_constant_size
            [...]/gcc/gimplify.c:664
    0x87e687 gimple_add_tmp_var(tree_node*)
            [...]/gcc/gimplify.c:702
    0x867f3d create_tmp_var(tree_node*, char const*)
            [...]/gcc/gimple-expr.c:476
    0x9a1b95 lower_omp_target
            [...]/gcc/omp-low.c:16875
    [...]


Grüße
 Thomas