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

Message ID f2f66ebf-c8fb-512c-1a1a-85010addf706@codesourcery.com
State New
Headers show

Commit Message

Cesar Philippidis April 20, 2017, 9:33 p.m.
On 04/20/2017 01:08 AM, Thomas Schwinge wrote:

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

Patch

2017-04-20  Cesar Philippidis  <cesar@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