Re: [PATCH, Fortran] Sync memory action delegated to OpenCoarrays
Totally fine with me. Thanks. 2015-03-10 0:20 GMT-07:00 Tobias Burnus : > Hi Alessandro, > > Alessandro Fanfarillo wrote: >> >> Thanks for the quick response. Patch built and regtested on >> x86_64-unknown-linux-gnu. > > > Actually, I also was about to send a reworked patch myself - see attachment. > Additional changes compared to your last patch: > * Add the assembler statement to libcaf_single also for the other syncs. > * Tested also the passing of the other arguments in the test case. > * Removed the call to sync_synchronize in the compiler - leaving it > completely to the library. (Existed for end of program, STOP and SYNC ...; > was only issued with -fcoarray=lib) > > Are the changes okay from your side? Then I'd prefer my patch - and I'll > commit it this evening. > > * * * > >> Currently the stub for sync memory in single.c invokes "asm volatile >> ("" : : : "memory")" but _gfortran_caf_sync_memory is not called when >> the code is compiled with -fcoarray=single. > > > I misremembered that we do call __sync_synchronize for -fcoarray=single. As > it turned out, we only call it for -fcoarray=lib - and it makes sense (in my > opinion) to leave such calls to the library. > > As you, I also added the "asm" to the library - which probably has no effect > - except when the library and the program are both compiled with LTO. Still, > it somehow looks more complete. > > > Regarding: Looks good to me - except that I prefer the additional items of > my patch - and that the ChangeLog doesn't conform to the GCC style: > - The file names shall be relative to the change log, i.e. "caf/single.c" > (as the ChangeLog file is in libgfortran/ChangeLog), similarly a > "gfortran.dg/" is missing for the test case. > - There should be a single "* " before the file names, not two tabs and > no "* ". > - After the file name, the changed symbols/function names shall be put in > parentheses. > - The item describing the change should end with a full stop. > - The email address should be enclosed in <>. > (Often missed, but not by you: date-name-email separation is two spaces.) > > Tobias > > >> Please let me know if I have to change the patch for -fcoarray=single. >> >> >> >> 2015-03-06 11:20 GMT-08:00 Tobias Burnus : >>> >>> Dear Alessandro, >>> >>> Alessandro Fanfarillo wrote: >>> >>> so far a "sync memory" statement is translated into a local >>> "__sync_synchronize ()". >>> The attached draft patch delegates the action for sync memory (when >>> -fcoarray=lib is used) to the external function >>> _gfortran_caf_sync_memory() implemented in the OpenCoarrays library. >>> >>> >>> Looks good to me. However, you should add a test case with 'dg-options >>> "-fdump-tree-original -fcoarray=lib"' to check that this works; cf. >>> gcc/testsuite/gfortran.dg/coarray*.f90 for examples. >>> >>> And you have to provide a stub implementation in >>> libgfortran/caf/{libcaf.h,single.c}. >>> >>> Tobias >>> >>> PS: I wonder whether it makes sense to remove the __sync_synchronize for >>> -fcoarray=single and replace it by the equivalent to "asm volatile ("" : >>> : : >>> "memory")". It almost certainly does. > >
Re: [PATCH, Fortran] Sync memory action delegated to OpenCoarrays
Hi Alessandro, Alessandro Fanfarillo wrote: Thanks for the quick response. Patch built and regtested on x86_64-unknown-linux-gnu. Actually, I also was about to send a reworked patch myself - see attachment. Additional changes compared to your last patch: * Add the assembler statement to libcaf_single also for the other syncs. * Tested also the passing of the other arguments in the test case. * Removed the call to sync_synchronize in the compiler - leaving it completely to the library. (Existed for end of program, STOP and SYNC ...; was only issued with -fcoarray=lib) Are the changes okay from your side? Then I'd prefer my patch - and I'll commit it this evening. * * * Currently the stub for sync memory in single.c invokes "asm volatile ("" : : : "memory")" but _gfortran_caf_sync_memory is not called when the code is compiled with -fcoarray=single. I misremembered that we do call __sync_synchronize for -fcoarray=single. As it turned out, we only call it for -fcoarray=lib - and it makes sense (in my opinion) to leave such calls to the library. As you, I also added the "asm" to the library - which probably has no effect - except when the library and the program are both compiled with LTO. Still, it somehow looks more complete. Regarding: Looks good to me - except that I prefer the additional items of my patch - and that the ChangeLog doesn't conform to the GCC style: - The file names shall be relative to the change log, i.e. "caf/single.c" (as the ChangeLog file is in libgfortran/ChangeLog), similarly a "gfortran.dg/" is missing for the test case. - There should be a single "* " before the file names, not two tabs and no "* ". - After the file name, the changed symbols/function names shall be put in parentheses. - The item describing the change should end with a full stop. - The email address should be enclosed in <>. (Often missed, but not by you: date-name-email separation is two spaces.) Tobias Please let me know if I have to change the patch for -fcoarray=single. 2015-03-06 11:20 GMT-08:00 Tobias Burnus : Dear Alessandro, Alessandro Fanfarillo wrote: so far a "sync memory" statement is translated into a local "__sync_synchronize ()". The attached draft patch delegates the action for sync memory (when -fcoarray=lib is used) to the external function _gfortran_caf_sync_memory() implemented in the OpenCoarrays library. Looks good to me. However, you should add a test case with 'dg-options "-fdump-tree-original -fcoarray=lib"' to check that this works; cf. gcc/testsuite/gfortran.dg/coarray*.f90 for examples. And you have to provide a stub implementation in libgfortran/caf/{libcaf.h,single.c}. Tobias PS: I wonder whether it makes sense to remove the __sync_synchronize for -fcoarray=single and replace it by the equivalent to "asm volatile ("" : : : "memory")". It almost certainly does. 2015-03-09 Alessandro Fanfarillo fanfarillo@gmail.com Tobias Burnus * trans.h (caf_sync_memory): New function decl tree. * trans-decl.c (gfc_build_builtin_function_decls): Define it. (create_main_function): Don't call sync_synchronize and leave it to the CAF library. * trans-stmt.c (gfc_trans_stop): Ditto. (gfc_trans_sync): Ditto; add call library call for sync memory. * coarray_sync_memory.f90: New. * caf/libcaf.h (_gfortran_caf_sync_memory): New prototype. * caf/single.c (_gfortran_caf_sync_memory): Implement. (_gfortran_caf_sync_all, _gfortran_caf_sync_image): Add __asm__ __volatile___ ("":::"memory"). diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index 3664824..769d487 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -153,6 +153,7 @@ tree gfor_fndecl_caf_get; tree gfor_fndecl_caf_send; tree gfor_fndecl_caf_sendget; tree gfor_fndecl_caf_sync_all; +tree gfor_fndecl_caf_sync_memory; tree gfor_fndecl_caf_sync_images; tree gfor_fndecl_caf_error_stop; tree gfor_fndecl_caf_error_stop_str; @@ -3451,6 +3452,10 @@ gfc_build_builtin_function_decls (void) get_identifier (PREFIX("caf_sync_all")), ".WW", void_type_node, 3, pint_type, pchar_type_node, integer_type_node); + gfor_fndecl_caf_sync_memory = gfc_build_library_function_decl_with_spec ( + get_identifier (PREFIX("caf_sync_memory")), ".WW", void_type_node, + 3, pint_type, pchar_type_node, integer_type_node); + gfor_fndecl_caf_sync_images = gfc_build_library_function_decl_with_spec ( get_identifier (PREFIX("caf_sync_images")), ".RRWW", void_type_node, 5, integer_type_node, pint_type, pint_type, @@ -5583,12 +5588,6 @@ create_main_function (tree fndecl) /* Coarray: Call _gfortran_caf_finalize(void). */ if (flag_coarray == GFC_FCOARRAY_LIB) { - /* Per F2008, 8.5.1 END of the main program implies a - SYNC MEMORY. */ - tmp = builtin_decl_explicit (BUILT_IN_SYNC_SYNCHRONIZE); - tmp = build_call_expr_loc (input_location, tmp, 0); - gfc_add_expr_to_block (&body, tmp); - tmp = build_call_expr_loc (input_loca
Re: [PATCH, Fortran] Sync memory action delegated to OpenCoarrays
Thanks for the quick response. Patch built and regtested on x86_64-unknown-linux-gnu. Currently the stub for sync memory in single.c invokes "asm volatile ("" : : : "memory")" but _gfortran_caf_sync_memory is not called when the code is compiled with -fcoarray=single. Please let me know if I have to change the patch for -fcoarray=single. 2015-03-06 11:20 GMT-08:00 Tobias Burnus : > Dear Alessandro, > > Alessandro Fanfarillo wrote: > > so far a "sync memory" statement is translated into a local > "__sync_synchronize ()". > The attached draft patch delegates the action for sync memory (when > -fcoarray=lib is used) to the external function > _gfortran_caf_sync_memory() implemented in the OpenCoarrays library. > > > Looks good to me. However, you should add a test case with 'dg-options > "-fdump-tree-original -fcoarray=lib"' to check that this works; cf. > gcc/testsuite/gfortran.dg/coarray*.f90 for examples. > > And you have to provide a stub implementation in > libgfortran/caf/{libcaf.h,single.c}. > > Tobias > > PS: I wonder whether it makes sense to remove the __sync_synchronize for > -fcoarray=single and replace it by the equivalent to "asm volatile ("" : : : > "memory")". It almost certainly does. 2015-03-09 Alessandro Fanfarillo fanfarillo@gmail.com trans-decl.c: Add function declaration caf_sync_memory trans.h: Ditto trans-stmt.c: Add caf_sync_memory invocation coarray_38.f90: Test case single.c: caf_sync_memory implementation libcaf.h: caf_sync_memory prototype diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index 3664824..a66c25c 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -153,6 +153,7 @@ tree gfor_fndecl_caf_get; tree gfor_fndecl_caf_send; tree gfor_fndecl_caf_sendget; tree gfor_fndecl_caf_sync_all; +tree gfor_fndecl_caf_sync_memory; tree gfor_fndecl_caf_sync_images; tree gfor_fndecl_caf_error_stop; tree gfor_fndecl_caf_error_stop_str; @@ -3451,6 +3452,10 @@ gfc_build_builtin_function_decls (void) get_identifier (PREFIX("caf_sync_all")), ".WW", void_type_node, 3, pint_type, pchar_type_node, integer_type_node); + gfor_fndecl_caf_sync_memory = gfc_build_library_function_decl_with_spec ( + get_identifier (PREFIX("caf_sync_memory")), ".WW", void_type_node, + 3, pint_type, pchar_type_node, integer_type_node); + gfor_fndecl_caf_sync_images = gfc_build_library_function_decl_with_spec ( get_identifier (PREFIX("caf_sync_images")), ".RRWW", void_type_node, 5, integer_type_node, pint_type, pint_type, diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c index 505f905..dce0e87 100644 --- a/gcc/fortran/trans-stmt.c +++ b/gcc/fortran/trans-stmt.c @@ -768,8 +768,7 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type) else stat = null_pointer_node; - if (code->expr3 && flag_coarray == GFC_FCOARRAY_LIB - && type != EXEC_SYNC_MEMORY) + if (code->expr3 && flag_coarray == GFC_FCOARRAY_LIB) { gcc_assert (code->expr3->expr_type == EXPR_VARIABLE); gfc_init_se (&argse, NULL); @@ -778,7 +777,7 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type) errmsg = gfc_build_addr_expr (NULL, argse.expr); errmsglen = argse.string_length; } - else if (flag_coarray == GFC_FCOARRAY_LIB && type != EXEC_SYNC_MEMORY) + else if (flag_coarray == GFC_FCOARRAY_LIB) { errmsg = null_pointer_node; errmsglen = build_int_cst (integer_type_node, 0); @@ -822,13 +821,13 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type) gfc_add_expr_to_block (&se.pre, tmp); } - if (flag_coarray != GFC_FCOARRAY_LIB || type == EXEC_SYNC_MEMORY) + if (flag_coarray != GFC_FCOARRAY_LIB) { /* Set STAT to zero. */ if (code->expr2) gfc_add_modify (&se.pre, stat, build_int_cst (TREE_TYPE (stat), 0)); } - else if (type == EXEC_SYNC_ALL) + else if (type == EXEC_SYNC_ALL || type == EXEC_SYNC_MEMORY) { /* SYNC ALL => stat == null_pointer_node SYNC ALL(stat=s) => stat has an integer type @@ -840,8 +839,13 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type) if (TREE_TYPE (stat) == integer_type_node) stat = gfc_build_addr_expr (NULL, stat); - tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_sync_all, -3, stat, errmsg, errmsglen); + if(type == EXEC_SYNC_MEMORY) + tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_sync_memory, + 3, stat, errmsg, errmsglen); + else + tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_sync_all, + 3, stat, errmsg, errmsglen); + gfc_add_expr_to_block (&se.pre, tmp); } else diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h index bd1520a..3ba2f88 100
Re: [PATCH, Fortran] Sync memory action delegated to OpenCoarrays
Dear Alessandro, Alessandro Fanfarillo wrote: so far a "sync memory" statement is translated into a local "__sync_synchronize ()". The attached draft patch delegates the action for sync memory (when -fcoarray=lib is used) to the external function _gfortran_caf_sync_memory() implemented in the OpenCoarrays library. Looks good to me. However, you should add a test case with 'dg-options "-fdump-tree-original -fcoarray=lib"' to check that this works; cf. gcc/testsuite/gfortran.dg/coarray*.f90 for examples. And you have to provide a stub implementation in libgfortran/caf/{libcaf.h,single.c}. Tobias PS: I wonder whether it makes sense to remove the __sync_synchronize for -fcoarray=single and replace it by the equivalent to "|asm volatile ("" : : : "memory")". It almost certainly does. |