-
Notifications
You must be signed in to change notification settings - Fork 85
Update ESMCI_BBox.C with volatile #502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
This update avoids an issue between `icpx` when ESMF is built with `ESMF_BOPT=O` and `ifx` when code *using* this is built with `-fpe0`.
|
I built this change in Baselibs with ifort 2021.13 (our production code) and then did a run of GEOS with and without. The runs were zero-diff. |
|
I know the Under #472 we were dealing with a section of the ESMF library that is just concerned with high level bookkeeping and labeling - not a performance critical part. However, I am concerned that what is proposed in #502 is actually touching performance critical portions of ESMF. I don't think we want to start adding |
theurich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes themselves look okay. However, I am concerned about performance impact as discussed in my longer comment.
|
@theurich Well there is this: which works just as well, though it might be non-zero-diff. But, since that (might?) replace 3 divides by one, it could be more performant (though maybe the But if you insist on not using this, then I can look at trying to add code into MAPL's CMake to prevent the use of |
|
I would be ok with this change. In either case it would likely make things faster to just do the divide once. Since the volatile is just used on the one local variable, I wonder if it would have much of an effect on performance.
… On Nov 6, 2025, at 2:16 PM, Matt Thompson ***@***.***> wrote:
mathomp4
left a comment
(esmf-org/esmf#502)
<#502 (comment)>
@theurich <https://github.com/theurich> Well there is this:
// (Optional but numerically nicer than sqrt of squares)
const double ns = std::hypot(std::hypot(nr[0], nr[1]), nr[2]);
// Only normalize if bigger than 0.0
if (ns > 1.0e-19) {
// Prevent speculative/hoisted reciprocal without vendor pragmas/flags
volatile double denom = ns; // portable optimization barrier
const double inv = 1.0 / denom; // executed only in the guarded block
nr[0] *= inv; nr[1] *= inv; nr[2] *= inv;
} else {
nr[0] = 0.0; nr[1] = 0.0; nr[2] = 0.0;
}
which works just as well, though it might be non-zero-diff. But, since that (might?) replace 3 divides by one, it could be more performant (though maybe the
But if you insist on not using this, then I can look at trying to add code into MAPL's CMake to prevent the use of -fpe0 in when we build with Debug flags. We'd probably have to do that otherwise we'd need to double our builds of ESMF every time since we know that ESMF_BOPT=g crushes regridding performance.
—
Reply to this email directly, view it on GitHub <#502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE6A7UYAJFN3E5IEUJ6N53T33O3B3AVCNFSM6AAAAACLEQTFYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIOJZGQZDANRQGY>.
You are receiving this because you were mentioned.
|
TL;DR: This update avoids an issue between
icpxwhen ESMF is built withESMF_BOPT=Oandifxwhen code using this is built with-fpe0.See GEOS-ESM/MAPL#4159 and GEOS-ESM/MAPL#4162
Crash in MAPL
This update is needed with an issue found by MAPL unit tests when trying out
ifx2025.3. The cause was if we build ESMF withESMF_BOPT=Obut then build MAPL withifxwith our Debug flags which have-fpe0. When we did this, we got a crash:where the call ends on an
ESMF_FieldRegridStore.Pure ESMF Reproducer
@bena-nasa then created a reproducer that was outside of MAPL and showed the same crash. With that reproducer, I was able to run with DDT and find that the offending code was:
from
esmf/src/Infrastructure/Mesh/src/Legacy/ESMCI_BBox.C
Lines 170 to 177 in c8ecfef
I'm guessing it is the divides in the if block.
Remove ESMF
I then asked ye olde Chat GPT for a small Fortran + C++ code that would exercise that chunk, and it seems to trigger it:
Chat GPT does have some thoughts about this code:
It suggested either extra flags when compiling with icpx:
or changing the code:
for doing the code bit.
I tested this and it does "fix" our issue And this is "nicer" code in that it removes three divides for one single one, etc. but the fear is this could be non-zero-diff.
But in talking with @oehmke, we decided to try a simpler test which is what this PR is where we go from:
to:
which seems to prevent the compiler from hoisting the code.
This change seems to fix our issues in MAPL.
I'll do a build with, say, our older production ifort 2021.13 to see if we get zero-diff with this code change. I think we should, but...