Hi!
On Wed, 19 Apr 2017 11:11:39 -0700, Cesar Philippidis <ce...@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
