Transcribe contents of interpolators_g.F from Fortran to C#160
Closed
mabruzzo wants to merge 14 commits into
Closed
Transcribe contents of interpolators_g.F from Fortran to C#160mabruzzo wants to merge 14 commits into
interpolators_g.F from Fortran to C#160mabruzzo wants to merge 14 commits into
Conversation
…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)
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.
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this pull request
Dec 26, 2024
Unlike other cases where we replaced ``logical`` with ``MASK_TYPE``, I replaced it with ``integer*8`` for the sake of consistency with the grackle-project#160 PR (I plan to use the transcription of interpolators_g.F from that PR in the future).
Collaborator
Author
|
This is now superseded by PR #251 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
EDIT: I refactored things quite since I first proposed this.1 It's ready to be reviewed now.
This PR is pretty self-explanatory. It transcribes the contents of
interpolators_g.Ffrom Fortran to C.interopsubdirectory ofsrc/clibUSE ISO_C_BINDING implicit NONE #include "grackle_fortran_types.def" #include "interop/interop_funcs.def"src/examples/utest_interpolatorsI include a number of unit-tests for each interpolator.-O3optimizations, all of the C and Fortran versions have extremely comparable performance.interpolate_1d_g. In this case, the C-version of the function appears to consistently be slightly (~5%) faster.So to summarize, there is basically no downside to this pull request.
Footnotes
Originally, this was build on top of PR Port
calc_temp1d_cloudy_g&cool1d_cloudy_gfrom Fortran to C #153 (since that was kinda the order in which I originally addressed things). Now PR Portcalc_temp1d_cloudy_g&cool1d_cloudy_gfrom Fortran to C #153 is built on top of this one. This PR is now stand-alone (and it should be reviewed before Portcalc_temp1d_cloudy_g&cool1d_cloudy_gfrom Fortran to C #153). The reason I swapped the order is that PR#153 likely involves a minor performance penalty (although I haven't carefully benchmarked that one) ↩