sourceryinstitute / OpenCoarrays

A parallel application binary interface for Fortran 2018 compilers.
http://www.opencoarrays.org
BSD 3-Clause "New" or "Revised" License
246 stars 56 forks source link

gfortran 7 regression: no type conversion before coarray put #292

Closed rouson closed 6 years ago

rouson commented 7 years ago

This issue is for tracking gfortran bug report 78892, which was first reported in an OpenCoarrays mailing list message. The gfortran bug report contains the following text:

The code below demonstrates a regression in which gfortran 7.0.0 20161215 is not performing a necessary implicit type conversion before putting data on a remote image.  By contrast, the code below works as expected with gfortran 5.4.0 and 6.2.0. 

$ cat convert-before-put.f90 
  real :: a[*]
  integer :: receiver
  associate(me=>this_image())
    if (me == 1) then
      do receiver = 2, num_images()
        a[receiver] = receiver ! implicit real(receiver) needed here
        sync images (receiver) ! notify remote image that data has been put
      end do 
    else
      sync images (1) ! await notification of data put by image 1
      if (a/=real(me)) print *, "Image ",me,": received ",a,", but expected ",real(me)
    end if
  end associate
end 

$ caf convert-before-put.f90 

$ cafrun -np 2 ./a.out
 Image            2 : received    0.00000000     , but expected    2.00000000
rouson commented 7 years ago

Tagging @egiovan to include him in the thread.

vehre commented 7 years ago

I still have to analyse this in-depth: But I am pretty sure, that the responsibility for the type-conversion is in the caf-library. There are convert-type routines that do the trick. In the put case gfortan might be able to do the type convert, but in the get case it can't. Therefore is it arguable that the responsibility is always with the library to be consistent.

vehre commented 7 years ago

At the moment I am adding pointer component support for derived typed coarrays, so please give a bit time to take a look.

rouson commented 7 years ago

@vehre, whichever side is responsible, this is a regression from the behavior with gfortran 5 and 6. In the interest of consistency, I assume it's best to keep the responsibility wherever it was prior to gfortran 7 unless there is a strong motivation for changing it. Just so we know how to assign this issue, please confirm whether you wrote the relevant library code that gets invoked when this example is compiled or whether @afanfa wrote it?

vehre commented 7 years ago

I just have rejected the pr in the gcc-bugtracker, because the responsibility for the type conversion was moved to the library. This way the library has the opportunity to reduce the number of bytes to transfer between the images where applicable. When the compiler does the type conversion, always the destination data type is used. With arrays this means the compiler generates a type conversion loop and the library "loops" again over the data to transfer it (I know you are using chunks and MPI-Datatypes to prevent single item transfer). Nevertheless the responsibility has been moved to the library. The conversion routine is already present (the _by_ref routine convert) so that it just needs to be called in the "old" communication primitives.

I am starting to implement it. Maybe @afanfa can check, that I don't mess up the performance completely.

vehre commented 7 years ago

The branch vehre/issue-292-type-conversion-during-communication has a preliminary version of a possible fix. It fixes the type conversion before put as long as the dest is contiguously addressed. Not type conversion support for sendget() and get() yet.

rouson commented 7 years ago

Thanks, @vehre ! Is there any chance you can join the Tuesday call this week to discuss this?

vehre commented 7 years ago

Sorry, will be traveling on Tuesday. Furthermore is this work in progress. You can go a long way with it, but not all to the end. I have seen that my testcase also raises ice-on-valid. Can someone have a look whether there is a PR in the gcc-bugtracker for that already?

On Sun, 10 Sep 2017 23:15:19 +0000 (UTC) Damian Rouson notifications@github.com wrote:

Thanks, @vehre ! Is there any chance you can join the Tuesday call this week to discuss this?

-- Andre Vehreschild Kreuzherrenstr. 8 52062 Aachen Tel.: +49 241 9291018 * Email: vehre@gmx.de

rouson commented 6 years ago

@vehre Please let us know if you'll have time to work on this soon. @zbeekman and I started work on new contract last month that requires the use of GCC 7. Fortunately, the project doesn't yet use coarrays, but coarrays will be a consideration later in the project so it would be great to get this issue resolved.

vehre commented 6 years ago

Hi Damian, My time is quite limited at the moment. I do have a better version with extended tests for send() already. But every time I add new testcases, I experience new compiler deficiencies or bugs in OpenCoarrays which makes progress quite tedious. So send is mostly complete for numeric types as long as one doesn't use copy to self (same image and same array). Furthermore had i not yet time to address the optimized strided communication. I will continue work as my time allows, which is mostly on the weekends.

Am 8. November 2017 18:40:31 MEZ schrieb Damian Rouson notifications@github.com:

@vehre Please let us know if you'll have time to work on this soon. @zbeekman and I started work on new contract last month that requires the use of GCC 7. Fortunately, the project doesn't yet use coarrays, but coarrays will be a consideration later in the project so it would be great to get this issue resolved.

vehre commented 6 years ago

Hi all, the latest commit to vehre/issue-292-... branch completes the type and kind conversion for pure put() operations. What is left is doing the same for get() and sendget(). But send() now can act as a template and should enable doing the other two quicker.

send()-features:

Note: The test send_convert_char_array is failing on travis, because gfortran has a bug. Gfortran >= 7.3 is needed to make the test pass.