Use two-pass IPO for OpenMP offload as well - #1880
Open
sbryngelson wants to merge 1 commit into
Open
Conversation
The NVHPC two-pass IPO (-Mextract/-Minline) was applied to OpenACC and
skipped wholesale for OpenMP offload, so the OpenMP lane gave up
cross-file inlining entirely. The reason turns out to be a single
compiler bug rather than anything structural:
nvfortran-Fatal-.../tools/fort2 TERMINATED by signal 11
gmake[3]: *** [.../m_data_output.fpp.f90.o] Error 2
fort2 segfaults inlining into m_data_output under OpenMP offload;
OpenACC compiles the same file cleanly. This is the same failure class
as the existing m_start_up/m_cbc exclusions -- OpenMP offload just
reaches it in one more place -- so it gets the same treatment, scoped
with MFC_OpenMP so the OpenACC build is unaffected.
With that one exclusion the OpenMP build completes with no crashes
(all four targets), and on A100 / NVHPC 25.11 a 1D sweep gives 175
passed and zero tolerance mismatches. The six cases that the OpenACC
IPO bug corrupted all pass here.
Worth weighing before merging: OpenMP offload has been the one NVHPC
GPU path not subject to this pass, and that independence is exactly
what identified the OpenACC miscompile -- omp passing 713/713 where acc
failed 23 is what ruled out the physics. Enabling IPO here buys
inlining on that lane at the cost of that cross-check, on a pass with a
track record of three separate defects, one of them silent. Reasonable
either way, but it should be a deliberate call and not a side effect.
Depends on nothing, but touches the same block as the
m_pressure_relaxation exclusion; whichever lands second needs a
one-line rebase.
Claude-Session: https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Enables NVHPC two-pass IPO (-Mextract/-Minline) for OpenMP offload builds while adding a targeted per-source exclusion to avoid a known fort2 crash during inlining.
Changes:
- Apply NVHPC two-pass IPO to OpenMP offload targets (previously skipped).
- Add conditional no-inline exclusion for
m_data_outputonly under the OpenMP offload configuration. - Refactor the no-inline list to allow conditional extensions cleanly.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
The NVHPC two-pass IPO (
-Mextract/-Minline) was applied to OpenACC and skipped wholesale for OpenMP offload, so that lane gave up cross-file inlining entirely. The reason turns out to be a single compiler bug rather than anything structural:fort2segfaults inlining intom_data_outputunder OpenMP offload. OpenACC compiles the same file cleanly.This is the same failure class as the existing
m_start_up/m_cbcexclusions — OpenMP offload just reaches it in one more place — so it gets the same treatment, scoped withMFC_OpenMPso the OpenACC build keeps full inlining there and loses nothing.Verification
On A100 / NVHPC 25.11:
fort2crashes(The 10
execution failedentries in my local sweep are a pre-existing Chemistry environment issue on this box — identical UUIDs in every configuration, including unmodified builds.)Only one extra exclusion was needed, not a cascade.
Worth weighing before merging
OpenMP offload has been the one NVHPC GPU path not subject to this pass, and that independence is exactly what identified the OpenACC miscompile in #1878 — omp passing 713/713 where acc failed 23 is what ruled out the physics and pointed at IPO.
Enabling IPO here buys inlining on that lane at the cost of that cross-check, on a pass with a track record of three separate defects, one of them silent. Reasonable either way, but it should be a deliberate call rather than a side effect of the fix — which is why it's split out.
If you'd rather keep the independent lane, closing this is a perfectly good outcome; the
fort2segfault documented above is worth keeping on record regardless, since it's the actual reason the exclusion existed.Merge note
Touches the same block as #1878's
m_pressure_relaxationexclusion. Whichever lands second needs a one-line rebase.https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg