Totally fine with me. Thanks.
2015-03-10 0:20 GMT-07:00 Tobias Burnus <bur...@net-b.de>: > 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 "<tab>* " 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 <bur...@net-b.de>: >>> >>> 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. > >