Convert itmask from logical to integer*4
#227
Open
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
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
Oct 29, 2024
This patch converts nearly all usages of the ``logical`` type to the
newly defined ``MASK_TYPE`` in the Fortran source files throughout
Grackle.
- The ``MASK_TYPE`` type is a custom datatype that just wraps a 32bit
integer.
- Because Fortran will not implicitly cast between logicals and
integers we need to explicitly compare this value against
``MASK_TRUE`` and ``MASK_FALSE``.
- For consistency with C semantics, if-statements that want to see
if the mask is true always compare ``MASK_TYPE`` value is not equal
to ``MASK_FALSE``.
- The **only** ``logical`` variable that wasn't changed was the
``end_int`` variable that we pass into ``interpolate_3Dz_g``.
We haven't touched this variable to avoid interfering with existing
efforts to transcribe that function to C/C++
- This patch is extremely similar in spirit to GH PR
grackle-project#227. But there a fewer minor distinctions:
- I am actually more confident in the correctness of this patch (I used
custom tools to automate the majority of these changes)
- I converted more variables in this patch (that other PR **only**
changed the ``itmask`` variable).
I have explicitly confirmed that all tests continue to pass after the
introduction of this PR
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.
Description
Prior to this patch, the
itmaskarray held elements of thelogicaldtype. This patch modified the Fortran layer so that the variable always has a dtype calledMASK_TYPE. In reality,MASK_TYPEis a macro that expands tointeger*4(the integer-size was an arbitrary choice).Motivation
As I previously shared during a development meeting, the usage of the
logicaldtype 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
logicaldtype and C's dtype. A crude workaround for this is to convert fromlogicaltointbefore a Fortran function calls a C function.This workaround is fine if you are converting a lone scalar
logicalargument (this is done in Transcribe contents ofinterpolators_g.Ffrom Fortran to C #160).However, it isn't ideal when you have to pass a
logicalarray, likeitmask, 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 Portcalc_temp1d_cloudy_g&cool1d_cloudy_gfrom Fortran to C #153). This is bad for performance since heap memory needs to be freshly allocated/deallocated off the heap during each function call. 1By now storing integers within
itmask(as is done by this PR), any transcribed C function now knows how to directly interpret the contents of the array.Alternative
The alternative to this patch would to declare
itmaskto be a variable with the typelogical(kind=c_bool), wherec_boolwould be provided by theISO_C_BINDINGFortran 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 asbool).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
_Booldtype may not have the same representation as C++'sbooldtype (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 chosen solution implemented by this PR is preferred.
To Summarize
This patch will allow us to simplify the proposed Fortran->C conversion in gh-#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
Footnotes
Let me be more concrete. In the absence of this patch, Port
calc_temp1d_cloudy_g&cool1d_cloudy_gfrom Fortran to C #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 thanitmask, directly onto the transcribed function. Instead of passingitmask, the "stub" function instead passes through a temporary array that it filled with coerced values fromitmask. Once this patch is merged, we can get rid of the "stub" functions and passitmaskdirectly to the transcribed function. ↩Aside: Transcribe contents of
interpolators_g.Ffrom Fortran to C #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. But these are very much an exception to the rule. (At this time, I don't think there are any other functions in the fortran layer that don't operate on an array of values). ↩