diff mbox

[Fortran] Memory sync after coarray image control statements and assignment

Message ID CAHqFgjXR5+y_=XXPTAxzjE0P2qq2NJiABnHd03zX-VfNfjuuGg@mail.gmail.com
State New
Headers show

Commit Message

Alessandro Fanfarillo Dec. 7, 2015, 2:48 p.m. UTC 
Your patch fixes the issues. In attachment patch, test case and changelog.

Thanks!

2015-12-07 11:06 GMT+01:00 Tobias Burnus <tobias.burnus@physik.fu-berlin.de>:
> I wrote:
>> I wonder whether using
>>
>> __asm__ __volatile__ ("":::"memory");
>>
>> would be sufficient as it has a way lower overhead than
>> __sync_synchronize().
>
> Namely, something like the attached patch.
>
> Regarding the original patch submission: Is there a reason that you didn't
> include the test case of Deepak from https://gcc.gnu.org/ml/fortran/2015-04/msg00062.html
> It should work as -fcoarray=lib -lcaf_single "dg-do run" test.
>
> Tobias
commit 69e650945454491bbaf81513a1eed10b5b6b0f45
Author: Alessandro Fanfarillo <fanfarillo@ing.uniroma2.it>
Date:   Mon Dec 7 15:46:10 2015 +0100

    Introducing __asm__ __volatile__ (:::memory) after image control statements, send and get
commit 3c36054cbeb23353d9fd81a9101861c812af35d5
Author: Alessandro Fanfarillo <fanfarillo@ing.uniroma2.it>
Date:   Mon Dec 7 15:15:45 2015 +0100

    Introducing __asm__ __volatile__ (:::memory) after image control statements, send and get

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index ba176a1..3f0cef0 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,15 @@
+2015-12-07  Tobias Burnus  <burnus@net-b.de>
+	    Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>
+
+	* trans.c (gfc_allocate_using_lib,gfc_deallocate_with_status):
+	Introducing __asm__ __volatile__ ("":::"memory")
+	after image control statements.
+	* trans-stmt.c 	(gfc_trans_sync,gfc_trans_event_post_wait)
+	(gfc_trans_lock_unlock): Ditto.
+	* trans-intrinsic.c (gfc_conv_intrinsic_caf_get,
+	conv_caf_send): Introducing __asm__ __volatile__ ("":::"memory")
+	after send, get and sendget.
+
 2015-12-05  Paul Thomas  <pault@gcc.gnu.org>
 
 	PR fortran/68676
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 188ed2b..0ac8836 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2015-12-07  Tobias Burnus  <burnus@net-b.de>
+	    Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>
+
+	* gfortran.dg/coarray_40.f90: New.
+
 2015-12-07  Kirill Yukhin  <kirill.yukhin@intel.com>
 
 	PR target/68627

Comments

Tobias Burnus Dec. 8, 2015, 10:01 a.m. UTC | #1 
Dear Alessandro, dear all,

On Mon, Dec 07, 2015 at 03:48:17PM +0100, Alessandro Fanfarillo wrote:
> Your patch fixes the issues. In attachment patch, test case and changelog.

Regarding the ChangeLog: Please include the added lines, only, and not the
change as patch. gcc/testsuite/ChangeLog changes too often such that a patch
won't apply.


Regarding the patch, I wonder where the memory synchronization makes sense
and where it is not required. (cf. also my email to Matthew in this thread,
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg00828.html)


I think it should be after all image control statements (8.5.1 in
http://j3-fortran.org/doc/year/15/15-007r2.pdf):
SYNC ALL, SYNC IMAGES, SYNC MEMORY, ALLOCATE, DEALLOCATE,
CRITICAL ... END CRITICAL, EVENT POST, EVENT WAIT, LOCK, UNLOCK,
MOVE_ALLOC.

Thus:
- SYNC ..., ALLOCATE/DEALLOCATE: I think those are all handled by the
  current patch
- MOVE_ALLOC: This one should be handled via the internal (de)allocate
  handling (please check!)
- EVENT WAIT, CRITICAL and LOCK: Obtaining a lock or receiving an event
  implies that quite likely some other process has changed something
  before. For those, the assembler statement really has to be added.
- EVENT POST, UNLOCK and END CRITICAL: While those are image control
  statements, I do not see how a remote image could modify a value in
  a well defined way, which is guaranteed to be available after that
  statement - but might not yet be available already at the previous
  segment (i.e. the previous image control statement).

Hence: I think you should update the patch to also handle 
EVENT WAIT, CRITICAL and LOCK - and to check MOVE_ALLOC.



Additionally, we need to handle the alias issue of:
  var = 5
  var[this_image()] = 42
  tmp = var

Both _gfortran_caf_send and _gfortran_caf_sendget can modify the
value of a variable; thus, calling the assembler after the function
makes sense.


However, _gfortran_caf_get does not modify the associated variable;
adding the assembler statement *after* _gfortran_caf_get. The
question is, however, whether one needs to take care of 'flushing'
the variable before the _gfortran_caf_get:
   var = 7
   ...
   var = 5
   tmp = var[this_image()]
   result = var + tmp
Here, one needs to prevent the compiler of keeping "5" only in a
register or moving "var = 5" after the _gfortran_caf_get call.


Thus, you have to move the assembler statement before the library
call in _gfortran_caf_get - and add another one at the beginning
of _gfortran_caf_sendget.

(For send/get, might might come up with something better than
""::"memory", but for now, it should do.)

Cheers,

Tobias
diff mbox

Patch

diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 21efe44..25ff311 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -1222,6 +1222,15 @@  gfc_conv_intrinsic_caf_get (gfc_se *se, gfc_expr *expr, tree lhs, tree lhs_kind,
   se->expr = res_var;
   if (array_expr->ts.type == BT_CHARACTER)
     se->string_length = argse.string_length;
+
+  /* It guarantees memory consistency within the same segment */
+  tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+  tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+		    gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+		    tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+  ASM_VOLATILE_P (tmp) = 1;
+  gfc_add_expr_to_block (&se->pre, tmp);
+
 }
 
 
@@ -1390,6 +1399,15 @@  conv_caf_send (gfc_code *code) {
   gfc_add_expr_to_block (&block, tmp);
   gfc_add_block_to_block (&block, &lhs_se.post);
   gfc_add_block_to_block (&block, &rhs_se.post);
+
+  /* It guarantees memory consistency within the same segment */
+  tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+  tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+		    gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+		    tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+  ASM_VOLATILE_P (tmp) = 1;
+  gfc_add_expr_to_block (&block, tmp);
+
   return gfc_finish_block (&block);
 }
 
diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c
index 3df483a..b7e1faa 100644
--- a/gcc/fortran/trans-stmt.c
+++ b/gcc/fortran/trans-stmt.c
@@ -818,6 +818,15 @@  gfc_trans_lock_unlock (gfc_code *code, gfc_exec_op op)
 				   errmsg, errmsg_len);
       gfc_add_expr_to_block (&se.pre, tmp);
 
+      /* It guarantees memory consistency within the same segment */
+      tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+      tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+			gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+			tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+      ASM_VOLATILE_P (tmp) = 1;
+
+      gfc_add_expr_to_block (&se.pre, tmp);
+
       if (stat2 != NULL_TREE)
 	gfc_add_modify (&se.pre, stat2,
 			fold_convert (TREE_TYPE (stat2), stat));
@@ -995,6 +1004,14 @@  gfc_trans_event_post_wait (gfc_code *code, gfc_exec_op op)
 			       errmsg, errmsg_len);
   gfc_add_expr_to_block (&se.pre, tmp);
 
+  /* It guarantees memory consistency within the same segment */
+  tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+  tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+		    gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+		    tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+  ASM_VOLATILE_P (tmp) = 1;
+  gfc_add_expr_to_block (&se.pre, tmp);
+
   if (stat2 != NULL_TREE)
     gfc_add_modify (&se.pre, stat2, fold_convert (TREE_TYPE (stat2), stat));
 
@@ -1080,6 +1097,18 @@  gfc_trans_sync (gfc_code *code, gfc_exec_op type)
 			       fold_convert (integer_type_node, images));
     }
 
+  /* Per F2008, 8.5.1, a SYNC MEMORY is implied by calling the
+     image control statements SYNC IMAGES and SYNC ALL.  */
+  if (flag_coarray == GFC_FCOARRAY_LIB)
+    {
+      tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+      tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+			gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+			tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+      ASM_VOLATILE_P (tmp) = 1;
+      gfc_add_expr_to_block (&se.pre, tmp);
+    }
+
   if (flag_coarray != GFC_FCOARRAY_LIB)
     {
       /* Set STAT to zero.  */
diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c
index 001db41..1993743 100644
--- a/gcc/fortran/trans.c
+++ b/gcc/fortran/trans.c
@@ -746,6 +746,14 @@  gfc_allocate_using_lib (stmtblock_t * block, tree pointer, tree size,
 			 TREE_TYPE (pointer), pointer,
 			 fold_convert ( TREE_TYPE (pointer), tmp));
   gfc_add_expr_to_block (block, tmp);
+
+  /* It guarantees memory consistency within the same segment */
+  tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+  tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+		    gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+		    tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+  ASM_VOLATILE_P (tmp) = 1;
+  gfc_add_expr_to_block (block, tmp);
 }
 
 
@@ -1356,6 +1364,14 @@  gfc_deallocate_with_status (tree pointer, tree status, tree errmsg,
 	     token, pstat, errmsg, errlen);
       gfc_add_expr_to_block (&non_null, tmp);
 
+      /* It guarantees memory consistency within the same segment */
+      tmp = gfc_build_string_const (strlen ("memory")+1, "memory"),
+      tmp = build5_loc (input_location, ASM_EXPR, void_type_node,
+			gfc_build_string_const (1, ""), NULL_TREE, NULL_TREE,
+			tree_cons (NULL_TREE, tmp, NULL_TREE), NULL_TREE);
+      ASM_VOLATILE_P (tmp) = 1;
+      gfc_add_expr_to_block (&non_null, tmp);
+
       if (status != NULL_TREE)
 	{
 	  tree stat = build_fold_indirect_ref_loc (input_location, status);
diff --git a/gcc/testsuite/gfortran.dg/coarray_40.f90 b/gcc/testsuite/gfortran.dg/coarray_40.f90
new file mode 100644
index 0000000..84af50e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/coarray_40.f90
@@ -0,0 +1,25 @@ 
+! { dg-do run }
+! { dg-options "-fcoarray=lib -lcaf_single" }
+!
+! Run-time test for memory consistency
+!
+! Contributed by Deepak Eachempati
+
+program cp_bug
+    implicit none
+    integer :: v1, v2, u[*]
+    integer :: me
+
+    me = this_image()
+
+    u = 0
+    v1 = 10
+
+    v1 = u[me]
+
+    ! v2 should get value in u (0)
+    v2 = v1
+
+    if(v2 /= u) call abort()
+
+end program