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.
>
>
