On 04/20/2017 01:08 AM, Thomas Schwinge wrote:
> On Wed, 19 Apr 2017 11:11:39 -0700, Cesar Philippidis
> <ce...@codesourcery.com> wrote:
>> 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 tried, but these were too closely related.
>> + 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?
This is only a compiler test and all of the interesting stuff happens
during gimplication. In fact, this test exposed an ICE while testing
allocatable scalars early on.
>> - 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?
Corrected in the attached gomp4-allocate-test.diff.
>> +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
> [...]
That's because lower_omp_target is now allocating local storage for
pointers, and that ICE is triggered by creating a temporary variable for
a VLA. I don't think VLAs are supported on accelerators because of the
lack of alloca, so I just fell back to the original behavior of not
allocating local storage for that variable. See gomp4-pr77371-1-ice.diff
for more details. Maybe the gimplifier should emit a warning when it
encounters such a variable? Another thing we can do is teach GCC how to
upload firstprivate variables into const memory, so that user cannot
manipulate them. But that doesn't help improve the correctness of the
program.
That lower_omp_target patch is still under test. I'll apply
gomp4-allocate-test.diff to gomp-4_0-branch shortly.
Cesar
2017-04-20 Cesar Philippidis <ce...@codesourcery.com>
gcc/
* omp-low.c (lower_omp_target): Don't privatize firstprivate VLAs.
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 5c41edc..cc209ba 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -16867,7 +16867,11 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
x = convert_from_firstprivate_int (x, TREE_TYPE (new_var),
is_reference (var),
&fplist);
- else if (is_reference (new_var))
+ /* Accelerators may not have alloca, so it's not
+ possible to privatize local storage for those
+ objects. */
+ else if (is_reference (new_var)
+ && TREE_CONSTANT (TYPE_SIZE (TREE_TYPE (var_type))))
{
/* Create a local object to hold the instance
value. */
2017-04-20 Cesar Philippidis <ce...@codesourcery.com>
libgomp/
* testsuite/libgomp.oacc-fortran/allocatable-scalar.f90: Clean up
test.
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90 b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90
index 8386c5d..4af42bc 100644
--- a/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90
+++ b/libgomp/testsuite/libgomp.oacc-fortran/allocatable-scalar.f90
@@ -1,3 +1,7 @@
+! Test non-declared allocatable scalars in OpenACC data clauses.
+
+! { dg-do run }
+
program main
implicit none
integer, parameter :: n = 100
@@ -19,7 +23,6 @@ program main
allocate (c)
- print *, loc (c)
!$acc parallel copyout(c) num_gangs(1)
c = a
!$acc end parallel
