[Fortran, Patch] Passing function pointer to co_reduce

[Fortran, Patch] Passing function pointer to co_reduce

[Alessandro Fanfarillo]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Dear all,

during the implementation of co_reduce in OpenCoarrays I noticed that
GFortran passes a pointer to function instead of the function name to
co_reduce.

Currently the compiler produces the following call:

_gfortran_caf_co_reduce (&desc.0, &simple_reduction, 0, 0, 0B, 0B, 0, 0);

where simple_reduction is the pure function that has to be used by co_reduce.

The attached patch seems to fix the issue, any comments?


Regards

Alessandro

PS: I also attach the test case

[-- Attachment #2: co_reduce_patch.diff --]
[-- Type: text/plain, Size: 778 bytes --]

commit a12b6ce6993df109c81a32d5684b4b9f41f69ea4
Author: Alessandro Fanfarillo <fanfarillo@ing.uniroma2.it>
Date:   Mon Jul 13 15:46:19 2015 +0200

    Fix function pointer argument for co_reduce

diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 66bc72a..967a741 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -8804,7 +8804,7 @@ conv_co_collective (gfc_code *code)
 	}
       opr_flags = build_int_cst (integer_type_node, opr_flag_int);
       gfc_conv_expr (&argse, opr_expr);
-      opr = gfc_build_addr_expr (NULL_TREE, argse.expr);
+      opr = argse.expr;
       fndecl = build_call_expr_loc (input_location, fndecl, 8, array, opr, opr_flags,
 				    image_index, stat, errmsg, strlen, errmsg_len);
     }

[-- Attachment #3: simple_reduce.f90 --]
[-- Type: text/x-fortran, Size: 359 bytes --]

program simple_reduce
  implicit none

  integer :: me

  me = this_image()

  sync all

  call co_reduce(me,simple_reduction)

  write(*,*) this_image(),me

contains
  
  pure function simple_reduction(a,b)
    integer,intent(in) :: a,b
    integer :: simple_reduction

    simple_reduction = a * b
  end function simple_reduction

end program simple_reduce

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Mikael Morin]

Hello,

Le 13/07/2015 17:31, Alessandro Fanfarillo a écrit :
> Dear all,
> 
> during the implementation of co_reduce in OpenCoarrays I noticed that
> GFortran passes a pointer to function instead of the function name to
> co_reduce.
> 
More exactly there are two address operators ('&') in a row (the dumping
removes one of them).

> Currently the compiler produces the following call:
> 
> _gfortran_caf_co_reduce (&desc.0, &simple_reduction, 0, 0, 0B, 0B, 0, 0);
> 
> where simple_reduction is the pure function that has to be used by co_reduce.
> 
> The attached patch seems to fix the issue, any comments?
> 
The patch itself looks good to me.
A ChangeLog entry should be provided with it.
The test is missing the usual dejagnu pattern matching directives to
check the generated code.
Do you have commit rights?

Mikael

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Damian Rouson]

> On Jul 15, 2015, at 8:58 AM, Mikael Morin <mikael.morin@sfr.fr> wrote:
> 
> The patch itself looks good to me.
> A ChangeLog entry should be provided with it.
> The test is missing the usual dejagnu pattern matching directives to
> check the generated code.
> Do you have commit rights?

Hi Mikael,

Thanks for the quick review.   I know Alessandro is not a reviewer. Assuming commit rights go with review rights, I’m guessing he doesn’t have commit rights.  I’ll let him confirm.  If he doesn’t have commit rights, could you commit it for us after he adds the derange pattern-matching directives?  Alternatively, if it’s easy, please feel free to add the directives and commit.  

Your fast response is very helpful to us.  The co_reduce collective is the final feature that we hope to finish before taking a snapshot of OpenCoarrays and designating it as our 1.0.0 release. :)   Toward that end, could this fix also be applied to the GCC 5 branch?  Soon after posting a tar ball of OpenCoarrays 1.0.0 on the web, Alessandro will submit a Portfile to the Macports developers that will enable OS X users to install OpenCoarrays 1.0.0 for use with GCC 5. It would be great for Macports users to have co_reduce when the next GCC 5 update releases.

Damian

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Mikael Morin]

Le 16/07/2015 16:34, Damian Rouson a écrit :
> Alternatively, if itÂ’s easy, please feel free to add the directives and commit.  
> 
It's not very difficult, but there is some pain getting the number of
escape backslashes right.
Never mind, I'll take care of it all.

Mikael

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

Le 17/07/2015 11:02, Mikael Morin a écrit :
> Le 16/07/2015 16:34, Damian Rouson a écrit :
>> Alternatively, if itÂ’s easy, please feel free to add the directives and commit.  
>>

> Never mind, I'll take care of it all.
> 
This is what I have committed:
https://gcc.gnu.org/r225930 (trunk)
https://gcc.gnu.org/r225932 (5 branch)

Mikael

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: r225930.diff --]
[-- Type: text/x-patch; name="r225930.diff", Size: 2443 bytes --]

Index: gcc/testsuite/gfortran.dg/co_reduce_1.f90
===================================================================
--- gcc/testsuite/gfortran.dg/co_reduce_1.f90	(révision 0)
+++ gcc/testsuite/gfortran.dg/co_reduce_1.f90	(révision 225930)
@@ -0,0 +1,33 @@
+! { dg-do compile }
+! { dg-additional-options "-fdump-tree-original -fcoarray=lib" }
+!
+! Check that we don't take twice the address of procedure simple_reduction
+! in the generated code.
+!
+! Contributed by Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>
+
+program simple_reduce
+  implicit none
+
+  integer :: me
+
+  me = this_image()
+
+  sync all
+
+  call co_reduce(me,simple_reduction)
+
+  write(*,*) this_image(),me
+
+contains
+  
+  pure function simple_reduction(a,b)
+    integer,intent(in) :: a,b
+    integer :: simple_reduction
+
+    simple_reduction = a * b
+  end function simple_reduction
+
+end program simple_reduce
+
+! { dg-final { scan-tree-dump "_gfortran_caf_co_reduce \\(&desc\\.\\d+,\\s*simple_reduction," "original" } }
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(révision 225929)
+++ gcc/testsuite/ChangeLog	(révision 225930)
@@ -1,3 +1,7 @@
+2015-07-17  Alessandro Fanfarillo  <fanfarillo.gcc@gmail.com>
+
+	* gfortran.dg/co_reduce_1.f90: New file.
+
 2015-07-17  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR target/66824
Index: gcc/fortran/ChangeLog
===================================================================
--- gcc/fortran/ChangeLog	(révision 225929)
+++ gcc/fortran/ChangeLog	(révision 225930)
@@ -1,3 +1,8 @@
+2015-07-17  Alessandro Fanfarillo  <fanfarillo.gcc@gmail.com>
+
+	* trans-intrinsic.c (conv_co_collective): Remove redundant address
+	operator in the generated code.
+
 2015-07-17  Andre Vehreschild  <vehre@gcc.gnu.org>
 
 	PR fortran/66035
Index: gcc/fortran/trans-intrinsic.c
===================================================================
--- gcc/fortran/trans-intrinsic.c	(révision 225929)
+++ gcc/fortran/trans-intrinsic.c	(révision 225930)
@@ -8804,7 +8804,7 @@
 	}
       opr_flags = build_int_cst (integer_type_node, opr_flag_int);
       gfc_conv_expr (&argse, opr_expr);
-      opr = gfc_build_addr_expr (NULL_TREE, argse.expr);
+      opr = argse.expr;
       fndecl = build_call_expr_loc (input_location, fndecl, 8, array, opr, opr_flags,
 				    image_index, stat, errmsg, strlen, errmsg_len);
     }

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Damian Rouson]

Thank you!

Damian

> On Jul 17, 2015, at 9:50 AM, Mikael Morin <mikael.morin@sfr.fr> wrote:
> 
> Le 17/07/2015 11:02, Mikael Morin a écrit :
>> Le 16/07/2015 16:34, Damian Rouson a écrit :
>>> Alternatively, if it’s easy, please feel free to add the directives and commit.  
>>> 
> 
>> Never mind, I'll take care of it all.
>> 
> This is what I have committed:
> https://gcc.gnu.org/r225930 (trunk)
> https://gcc.gnu.org/r225932 (5 branch)
> 
> Mikael
> <r225930.diff>

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Alessandro Fanfarillo]

Thanks a lot!

2015-07-17 15:50 GMT+02:00 Mikael Morin <mikael.morin@sfr.fr>:
> Le 17/07/2015 11:02, Mikael Morin a écrit :
>> Le 16/07/2015 16:34, Damian Rouson a écrit :
>>> Alternatively, if it’s easy, please feel free to add the directives and commit.
>>>
>
>> Never mind, I'll take care of it all.
>>
> This is what I have committed:
> https://gcc.gnu.org/r225930 (trunk)
> https://gcc.gnu.org/r225932 (5 branch)
>
> Mikael

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Steve Kargl]

On Fri, Jul 17, 2015 at 03:50:26PM +0200, Mikael Morin wrote:
> Le 17/07/2015 11:02, Mikael Morin a écrit :
> > Le 16/07/2015 16:34, Damian Rouson a écrit :
> >> Alternatively, if it?s easy, please feel free to add the directives and commit.  
> >>
> 
> > Never mind, I'll take care of it all.
> > 
> This is what I have committed:
> https://gcc.gnu.org/r225930 (trunk)
> https://gcc.gnu.org/r225932 (5 branch)
> 

Hi Mikael,

It seems that this commit has changed the scanning of 
other testcases.

FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&desc.., &fr, 4, _gfortran_caf_num_images \\(0, -1\\), &stat1, errmesg1, 0, 6\\);" 1
FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&val2, &gz, 0, 4, &stat2, errmesg2, 0, 7\\);" 1
FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&desc.., &hc, 1, res, &stat3, errmesg3, 99, 8\\);"

-- 
Steve

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Mikael Morin]

Le 17/07/2015 20:40, Steve Kargl a écrit :
> On Fri, Jul 17, 2015 at 03:50:26PM +0200, Mikael Morin wrote:
>> Le 17/07/2015 11:02, Mikael Morin a écrit :
>>> Le 16/07/2015 16:34, Damian Rouson a écrit :
>>>> Alternatively, if it?s easy, please feel free to add the directives and commit.  
>>>>
>>
>>> Never mind, I'll take care of it all.
>>>
>> This is what I have committed:
>> https://gcc.gnu.org/r225930 (trunk)
>> https://gcc.gnu.org/r225932 (5 branch)
>>
> 
> Hi Mikael,
> 
> It seems that this commit has changed the scanning of 
> other testcases.
> 
> FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&desc.., &fr, 4, _gfortran_caf_num_images \\(0, -1\\), &stat1, errmesg1, 0, 6\\);" 1
> FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&val2, &gz, 0, 4, &stat2, errmesg2, 0, 7\\);" 1
> FAIL: gfortran.dg/coarray_collectives_16.f90   -O   scan-tree-dump-times original "_gfortran_caf_co_reduce \\(&desc.., &hc, 1, res, &stat3, errmesg3, 99, 8\\);"
> 
Hmm,

now you bring the topic, I realize I haven't tested the patch against
the testsuite (except the new file).
Will fix.

Mikael

Re: [Fortran, Patch] Passing function pointer to co_reduce

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

Le 17/07/2015 21:39, Mikael Morin a écrit :
>> It seems that this commit has changed the scanning of 
>> other testcases.
>>
> Will fix.
> 
Done:
https://gcc.gnu.org/r225965 (trunk)
https://gcc.gnu.org/r225969 (5 branch)

Mikael



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: r225965.diff --]
[-- Type: text/x-patch; name="r225965.diff", Size: 1615 bytes --]

Index: gcc/testsuite/gfortran.dg/coarray_collectives_16.f90
===================================================================
--- gcc/testsuite/gfortran.dg/coarray_collectives_16.f90	(révision 225964)
+++ gcc/testsuite/gfortran.dg/coarray_collectives_16.f90	(révision 225965)
@@ -33,6 +33,6 @@ contains
   end function hc
 end program test
 
-! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&desc.., &fr, 4, _gfortran_caf_num_images \\(0, -1\\), &stat1, errmesg1, 0, 6\\);" 1 "original" } }
-! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&val2, &gz, 0, 4, &stat2, errmesg2, 0, 7\\);" 1 "original" } }
-! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&desc.., &hc, 1, res, &stat3, errmesg3, 99, 8\\);" 1 "original" } }
+! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&desc.., fr, 4, _gfortran_caf_num_images \\(0, -1\\), &stat1, errmesg1, 0, 6\\);" 1 "original" } }
+! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&val2, gz, 0, 4, &stat2, errmesg2, 0, 7\\);" 1 "original" } }
+! { dg-final { scan-tree-dump-times "_gfortran_caf_co_reduce \\(&desc.., hc, 1, res, &stat3, errmesg3, 99, 8\\);" 1 "original" } }
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(révision 225964)
+++ gcc/testsuite/ChangeLog	(révision 225965)
@@ -1,3 +1,8 @@
+2015-07-17  Mikael Morin  <mikael@gcc.gnu.org>
+
+	* gfortran.dg/coarray_collectives_16.f90: Fix pattern
+	as follow-up to r225930.
+
 2015-07-17  Paul Thomas  <pault@gcc.gnu.org>
 
 	PR fortran/52846