scorpio icon indicating copy to clipboard operation
scorpio copied to clipboard

Handling of real kind in chunk_cache routines in pio_nf.F90

Open bartgol opened this issue 5 years ago • 6 comments

There are several instances in the Scorpio Fortran interface source code where Fortran types without kind specifications are passed directly to the C interface functions. The types of the arguments match when default kind specifications are used for the types (e.g. default kind for a REAL is REAL*4 - 4 bytes, this type is compatible with a REAL(C_FLOAT) ). However they do not match, and results in a compiler error, when the default kind of the types are changed (e.g. when a user specifies via compiler flags that by default REALs without a specific KIND specification need to be 8 bytes) .

An example:

  integer function set_chunk_cache(iosysid, iotype, chunk_cache_size, chunk_cache_nelems, &
       chunk_cache_preemption) result(ierr)
    integer, intent(in) :: iosysid
    integer, intent(in) :: iotype
    integer(kind=PIO_OFFSET_KIND), intent(in) :: chunk_cache_size
    integer(kind=PIO_OFFSET_KIND), intent(in) :: chunk_cache_nelems
    real, intent(in) :: chunk_cache_preemption

    interface
       integer (C_INT) function PIOc_set_chunk_cache(iosysid, iotype, chunk_cache_size, &
            chunk_cache_nelems, chunk_cache_preemption) &
            bind(c,name="PIOc_set_chunk_cache")
         use iso_c_binding
         integer(c_int), value :: iosysid
         integer(c_int), value :: iotype
         integer(c_size_t), value :: chunk_cache_size
         integer(c_size_t), value :: chunk_cache_nelems
         real(c_float), value :: chunk_cache_preemption
       end function PIOc_set_chunk_cache
    end interface

    ierr = PIOc_set_chunk_cache(iosysid, iotype, chunk_cache_size, chunk_cache_nelems, &
         chunk_cache_preemption)
  end function set_chunk_cache

The interface function expects a c_float (i.e., kind=4). but the variable passed to it has unspecified kind. If the user asks the compiler to default reals to kind=8, this generates a compiler error:

/home/luca/workdir/scream/scream-src/branch/externals/scorpio/src/flib/pio_nf.F90:1734:11:

     ierr = PIOc_get_chunk_cache(iosysid, iotype, chunk_cache_size, chunk_cache_nelems, &
           1
Error: Type mismatch in argument ‘chunk_cache_preemption’ at (1); passed REAL(8) to REAL(4)

For the set_blah functions, one can simply cast on the fly, like this

    ierr = PIOc_set_chunk_cache(iosysid, iotype, chunk_cache_size, chunk_cache_nelems, &
         real(chunk_cache_preemption,kind=c_float))

while for the get functions, I think you need a temporary:

    real(kind=c_float) :: tmp
    ierr = PIOc_get_chunk_cache(iosysid, iotype, chunk_cache_size, chunk_cache_nelems, &
         tmp)
    chunk_cache_preemption = tmp

I can put up a quick PR, if this seems reasonable.

bartgol avatar Sep 24 '20 22:09 bartgol

Is this an issue that you typically encounter in your runs (Do you try runs with 8 byte reals? I want to understand how critical this issue is to your workflow.)?

We might need to look at all the APIs to see if we can support these compiler options (switching the default size of types). Note that the user could switch the default size for integers too.

A quick fix for the user would be to build Scorpio without the flags (since it currently does not support it) and always pass 4 byte reals to the library where reals are expected.

jayeshkrishna avatar Sep 25 '20 01:09 jayeshkrishna

In SCREAM, we support both single and double precision builds. To make things simpler, in double precision we enable the flags that make fortran default reals to double precision. It is a bit hard to "remove" a flag once it's added to the flags; doable, but a bit of a pain, so it would be nice if scorpio wasn't affected by the presence (or lack) of this flag.

From what you say, I take it that scorpio only supports 4 byte reals, right? If so, why not enforcing all reals to have kind=4 (which is compatible to c_float)? Leaving inputs delcared as real :: something leaves room for subtle errors (or compiler error, like this).

bartgol avatar Sep 25 '20 02:09 bartgol

We need to handle this case (user changing default size of the types) if its required by SCREAM. I wanted to get a feel of how urgent the requirement is, also like I noted above users could change the default size of other types too and hence this change is bigger and needs a closer look.

jayeshkrishna avatar Sep 25 '20 03:09 jayeshkrishna

It might be that you can simply add the correct flag to the compiler flags of scorpio. Hopefully, last flag overrides previous ones. But it seems a bit fragile. Hardcoding the kind of each numeral/logical seems more solid. Also, if scorpio one day wants to change its kinds, it would only have to change the single place where the universal kind is specified.

bartgol avatar Sep 25 '20 03:09 bartgol

Fortran routines should explicitly type their reals so they're immune to any flags that try to autopromote reals. For API that accept a real array, this means you may have to provide 2 versions. Modern Fortran is supposed to support templating for that but I've never tried it.

rljacob avatar Sep 25 '20 04:09 rljacob

We found that PGI does not respect the "last flag overrides previous conflicting flags" rule.

rljacob avatar Sep 25 '20 04:09 rljacob