Port calc_temp1d_cloudy_g & cool1d_cloudy_g from Fortran to C
#153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mabruzzo
changed the title
calc_temp1d_cloudy_g from Fortran to Ccalc_temp1d_cloudy_g & cool1d_cloudy_g from Fortran to C
mabruzzo
changed the title
calc_temp1d_cloudy_g & cool1d_cloudy_g from Fortran to Ccalc_temp1d_cloudy_g & cool1d_cloudy_g from Fortran to C
…eger*8 Previously end_int was a logical. This produced issues while trying to port calc_temp1d_cloudy_g to C. It's unclear whether integer*8 is the best type, but it's a place to start
…test. The transcribed routines are not currently called within the main Grackle library
- remove last reference to gr_dint (it's equivalent to gr_int) - include the relevant header - add file heading
…ch about performance in this case)
This was done to circumvent some issues with transcribing that subroutine to C. Specifically, the LOGICAL dtype is incompatible with C, unless we explicitly use LOGICAL(KIND=C_BOOL) everywhere. Since this may also produce issues with C++ (the bool type in C++ and the _Bool type introduced in C99 may not be compatible), we have opted to replace it with an integer type. In practice, the only change we needed to make to calc_temp1d_cloudy_g's signature was to alter the type of itmask. To make this all work, without changing large regions of the codebase, we have introduced a wrapper function called calc_temp1d_cloudy_shim_g.F that explicitly converts the dtype from LOGICAL to INTEGER of itmask. This introduces an extra allocation, but I'm skeptical it will make a huge difference in speed (at least right now - as the code is currently written)
The old fortran version has been retained as a unit-test.
…temp1d_cloudy_g, rather than the shim.
…2bit ints This is done to help with transcription of the routine to C
…rsion is still used)
…t (the main library now makes use of the C implementation).
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this pull request
Jul 17, 2024
Description
-----------
Prior to this patch, the ``itmask`` array held elements of the
``logical`` dtype. This patch modified the Fortran layer so that the
variable always has a dtype called ``MASK_TYPE``. In reality,
``MASK_TYPE`` is a macro that expands to ``integer*4`` (the integer-size
was an arbitrary choice).
Motivation
----------
As I previously shared during a development meeting, the usage of the
``logical`` dtype is a bit of an issue for some Fortran->C transcription
efforts. The issue is that:
* the Fortran specification doesn't define the representation for
``.true.`` and ``.false.`` values. Consequently, different
implementations make different choices (as I understand it, gfortran
and ifort originally made different choices -- ifort only changed
their mind in the last 10 years).
* importantly, this means that there isn't a clear conversion from
Fortan's ``logical`` dtype and C's dtype. A crude workaround for
this is to convert from ``logical`` to ``int`` before a Fortran
function calls a C function.
* This works fine for a scalar ``logical`` argument (this is what
gh-grackle-project#160 does).
* However, it isn't ideal when you have to pass ``logical`` array,
like ``itmask``, to a C function. You ultimately need to allocate
a temporary intermediate array and pass it through to the C function
(as is done by gh-grackle-project#153). This is bad for performance since heap
memory needs to be freshly allocated/deallocated off the heap during
each function call. [^1]
By now directly storing integers within ``itmask``, any transcribed C
function now knows how to directly interpret the contents of the array.
Alternative
-----------
The alternative to this patch would to declare ``itmask`` to be a
variable with the type ``logical(kind=c_bool)``, where ``c_bool`` would
be provided by the ``ISO_C_BINDING`` Fortran module (formally, the
module was introduced with Fortran 2003, but all major fortran compilers
support it in other modes). Essentially, this would tell the Fortran
compiler to represent it in the same way as the boolean dtype
introduced to C in 1999 (it was formally called ``_Bool``, but macros
were provided so that you could refer to it as ``bool``).
This alternative would have been less invasive. However, there could be
issues if we ever decided to compile Grackle with a C++ compiler. These
issues could arise because C's ``_Bool`` dtype may not have the same
representation as C++'s ``bool`` dtype (I think this is mostly an issue
when you have compilers provided by different vendors). In order to
support GPUs, Grackle will probably need to start using C++ compilers
(even if we just restrict to the common subset of the language that is
shared with C).
At a previous development meeting, it was decided that the choosen
solution implemented by this patch is preferred.
To Summarize
------------
This patch will allow us to simplify the proposed Fortran->C conversion
in gh-grackle-project#153 (and it will removed performance-overhead of the extra
heap-allocations currently required in the absence of this patch).
In the same way, this patch will **significantly** simplify the process
of converting converting any Fortran functionality that takes an iteration
mask and operates on a slice. This is the vast majority of all grackle
functionality! [^2]
[^1]: Let me be more concrete. In the absence of this patch, gh-grackle-project#153
currently introduces "stub" functions for each transcribed function. Each
"stub" function takes all of the arguments for the transcribed function.
The "stub" function's only purpose is to forward all of the arguments,
other than ``itmask``, directly onto the transcribed function. Instead
of passing ``itmask``, the "stub" function instead passes through a
temporary array that it filled with coerced values from ``itmask``. With
this patch,
allocate a temporary array,
(ii) fills that temporary array
that temporary ``itmask``
[^2]: Aside: gh-grackle-project#160 shows a notable example where this patch isn't
particularly relevant, since that PR converts a set of functions that
only operate on a single value rather than on an array of values.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ports
calc_temp1d_cloudy_gandcool1d_cloudy_gfrom Fortran to C.EDIT: I have now refactored this PR so that it builds off of #160 (In other words, #160 must be reviewed first).
In each case, I put the new C routine in a new subdirectory of
clibcalledinterop. Let me know if you dislike this, but my thinking was that it would be useful to put the transcribed routines in a special place so that they could serve as an example for how to do it in the future. There's also a ReadMe file in that directory explaining how to call the routine from both C and Fortran.If you look through the commit history, I first made a one-to-one transcription and then I did some refactoring of the C version (to drop some unnecessary heap allocations). I also introduced new unit tests that directly compares the new version of the code against the old version. As far as I can tell from my tests, we get bitwise identical results (I suspect that extreme values that could break the math library functions).
I also played around with transcribing
calc_temp_cloudy_g.F(since it's really easy to tweak my unit tests to apply to these cases). But, I'm not currently confident in their accuracy so I haven't included them.