Hi,
in attachment the new patch. I also checked the behavior with
move_alloc: it synchronizes right after the deregistration of the
destination.
I also noticed that __asm__ __volatile__ ("":::"memory") is called
before sync all and not after. It shouldn't be a problem, right?
2015-12-08 11:01 GMT+01:00 Tobias Burnus <[email protected]>:
> 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
2015-12-08 Tobias Burnus <[email protected]>
Alessandro Fanfarillo <[email protected]>
* 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, gfc_trans_critical): Ditto.
* trans-intrinsic.c (gfc_conv_intrinsic_caf_get,
conv_caf_send): Introducing __asm__ __volatile__ ("":::"memory")
after send, before get and around sendget.
2015-12-08 Tobias Burnus <[email protected]>
Alessandro Fanfarillo <[email protected]>
* gfortran.dg/coarray_40.f90: New.
commit 6cdffc285931eb604d4c900d77fe60b96d604556
Author: Alessandro Fanfarillo <[email protected]>
Date: Tue Dec 8 14:58:20 2015 +0100
Introducing __asm__ __volatile__ (:::memory) after image control
statements, send, get and sendget
diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 21efe44..0e4fcc5 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -1211,6 +1211,14 @@ gfc_conv_intrinsic_caf_get (gfc_se *se, gfc_expr *expr,
tree lhs, tree lhs_kind,
if (lhs == NULL_TREE)
may_require_tmp = boolean_false_node;
+ /* 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);
+
tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_get, 9,
token, offset, image_index, argse.expr, vec,
dst_var, kind, lhs_kind, may_require_tmp);
@@ -1222,6 +1230,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); */
+
}
@@ -1375,6 +1392,14 @@ conv_caf_send (gfc_code *code) {
{
tree rhs_token, rhs_offset, rhs_image_index;
+ /* 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);
+
caf_decl = gfc_get_tree_for_caf_expr (rhs_expr);
if (TREE_CODE (TREE_TYPE (caf_decl)) == REFERENCE_TYPE)
caf_decl = build_fold_indirect_ref_loc (input_location, caf_decl);
@@ -1390,6 +1415,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..0075ae4 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. */
@@ -1401,6 +1430,15 @@ gfc_trans_critical (gfc_code *code)
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);
+
tmp = gfc_trans_code (code->block->next);
gfc_add_expr_to_block (&block, tmp);
@@ -1413,6 +1451,14 @@ gfc_trans_critical (gfc_code *code)
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);
return gfc_finish_block (&block);
}
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
