diff mbox

[openacc] implicit non-scalars data mapping in kernels

Message ID 56982904.60507@codesourcery.com
State New
Headers show

Commit Message

Cesar Philippidis Jan. 14, 2016, 11:02 p.m. UTC 
On 01/14/2016 06:18 AM, Nathan Sidwell wrote:
> On 01/13/16 17:44, Jakub Jelinek wrote:
>> On Wed, Jan 13, 2016 at 11:35:08PM +0100, Marek Polacek wrote:
>>> On Wed, Jan 13, 2016 at 02:29:21PM -0800, Cesar Philippidis wrote:
>>>> --- a/gcc/gimplify.c
>>>> +++ b/gcc/gimplify.c
>>>> @@ -5994,6 +5994,11 @@ oacc_default_clause (struct gimplify_omp_ctx
>>>> *ctx, tree decl, unsigned flags)
>>>>   {
>>>>     const char *rkind;
>>>>     bool on_device = false;
>>>> +  tree type = TREE_TYPE (decl);
>>>> +
>>>> +  if (TREE_CODE (type) == REFERENCE_TYPE
>>>> +      || POINTER_TYPE_P (type))
>>>
>>> I think this should be just POINTER_TYPE_P--this macro checks for
>>> REFERENCE_TYPE as well.
>>
>> Unless the spec says that for all pointers you should consider what it
>> points to, this really looks weird to me.
> 
> I think the spec is silent.  I remember looking at this before and
> thinking looking through pointers was strange.  IMHO it should behave in
> a manner as similar as possible to OpenMP (which I presume is precise
> about whether one should look through arbitrary pointer types).
> 
> Is this a case of the Fortran FE OpenACC bits not annotating some
> artificial object correctly?

I don't think it's specific to fortran. You can have reference types in
c++ too. Actually, Jakub's comment reminded me to create a test case for
c++, and it turns out that the c++ FE does not like any reference types
in openacc data clauses at all. At this point, I'm reasonably confident
that this issue is independent from the c++ issue.

Is this patch ok for trunk? C++ support is still a work in progress, and
I'll address that in a follow up patch.

Cesar

Comments

Jakub Jelinek Jan. 14, 2016, 11:10 p.m. UTC | #1 
On Thu, Jan 14, 2016 at 03:02:28PM -0800, Cesar Philippidis wrote:
> > Is this a case of the Fortran FE OpenACC bits not annotating some
> > artificial object correctly?
> 
> I don't think it's specific to fortran. You can have reference types in
> c++ too. Actually, Jakub's comment reminded me to create a test case for
> c++, and it turns out that the c++ FE does not like any reference types
> in openacc data clauses at all. At this point, I'm reasonably confident
> that this issue is independent from the c++ issue.
> 
> Is this patch ok for trunk? C++ support is still a work in progress, and
> I'll address that in a follow up patch.

LGTM, so if Nathan is ok with that too, it is ok for trunk.

> 2016-01-14  Cesar Philippidis  <cesar@codesourcery.com>
> 
> 	gcc/
> 	* gimplify.c (oacc_default_clause): Decode reference and pointer
> 	types for both kernels and parallel regions.
> 
> 	libgomp/
> 	* testsuite/libgomp.oacc-fortran/kernels-data.f90: New test.

	Jakub
Nathan Sidwell Jan. 15, 2016, 1:54 p.m. UTC | #2 
On 01/14/16 18:10, Jakub Jelinek wrote:
> On Thu, Jan 14, 2016 at 03:02:28PM -0800, Cesar Philippidis wrote:
>>> Is this a case of the Fortran FE OpenACC bits not annotating some
>>> artificial object correctly?
>>
>> I don't think it's specific to fortran. You can have reference types in
>> c++ too. Actually, Jakub's comment reminded me to create a test case for
>> c++, and it turns out that the c++ FE does not like any reference types
>> in openacc data clauses at all. At this point, I'm reasonably confident
>> that this issue is independent from the c++ issue.
>>
>> Is this patch ok for trunk? C++ support is still a work in progress, and
>> I'll address that in a follow up patch.
>
> LGTM, so if Nathan is ok with that too, it is ok for trunk.

Ok too, thanks!
diff mbox

Patch

2016-01-14  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* gimplify.c (oacc_default_clause): Decode reference and pointer
	types for both kernels and parallel regions.

	libgomp/
	* testsuite/libgomp.oacc-fortran/kernels-data.f90: New test.


diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 17144d1..eda2e9c 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -5994,6 +5994,10 @@  oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags)
 {
   const char *rkind;
   bool on_device = false;
+  tree type = TREE_TYPE (decl);
+
+  if (lang_hooks.decls.omp_privatize_by_reference (decl))
+    type = TREE_TYPE (type);
 
   if ((ctx->region_type & (ORT_ACC_PARALLEL | ORT_ACC_KERNELS)) != 0
       && is_global_var (decl)
@@ -6012,7 +6016,7 @@  oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags)
       /* Scalars are default 'copy' under kernels, non-scalars are default
 	 'present_or_copy'.  */
       flags |= GOVD_MAP;
-      if (!AGGREGATE_TYPE_P (TREE_TYPE (decl)))
+      if (!AGGREGATE_TYPE_P (type))
 	flags |= GOVD_MAP_FORCE;
 
       rkind = "kernels";
@@ -6020,12 +6024,6 @@  oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags)
 
     case ORT_ACC_PARALLEL:
       {
-	tree type = TREE_TYPE (decl);
-
-	if (TREE_CODE (type) == REFERENCE_TYPE
-	    || POINTER_TYPE_P (type))
-	  type = TREE_TYPE (type);
-
 	if (on_device || AGGREGATE_TYPE_P (type))
 	  /* Aggregates default to 'present_or_copy'.  */
 	  flags |= GOVD_MAP;
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/non-scalar-data.f90 b/libgomp/testsuite/libgomp.oacc-fortran/non-scalar-data.f90
new file mode 100644
index 0000000..4afb562
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-fortran/non-scalar-data.f90
@@ -0,0 +1,50 @@ 
+! Ensure that a non-scalar dummy arguments which are implicitly used inside
+! offloaded regions are properly mapped using present_or_copy.
+
+! { dg-do run }
+
+program main
+  implicit none
+
+  integer, parameter :: n = 100
+  integer :: array(n), i
+  
+  !$acc data copy(array)
+  call kernels(array, n)
+
+  !$acc update host(array)
+
+  do i = 1, n
+     if (array(i) .ne. i) call abort
+  end do
+
+  call parallel(array, n)
+  !$acc end data
+
+  do i = 1, n
+     if (array(i) .ne. i+i) call abort
+  end do
+end program main
+
+subroutine kernels (array, n)
+  integer, dimension (n) :: array
+  integer :: n, i
+
+  !$acc kernels
+  do i = 1, n
+     array(i) = i
+  end do
+  !$acc end kernels
+end subroutine kernels
+
+
+subroutine parallel (array, n)
+  integer, dimension (n) :: array
+  integer :: n, i
+
+  !$acc parallel
+  do i = 1, n
+     array(i) = i+i
+  end do
+  !$acc end parallel
+end subroutine parallel