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 <tobias.bur...@physik.fu-berlin.de>: > 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 <bur...@net-b.de> Alessandro Fanfarillo <fanfarillo....@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, 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 <bur...@net-b.de> Alessandro Fanfarillo <fanfarillo....@gmail.com> * gfortran.dg/coarray_40.f90: New. commit 6cdffc285931eb604d4c900d77fe60b96d604556 Author: Alessandro Fanfarillo <fanfari...@ing.uniroma2.it> 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