Stop NVHPC IPO silently miscompiling the six-equation relaxation kernel - #1878
Open
sbryngelson wants to merge 1 commit into
Open
Stop NVHPC IPO silently miscompiling the six-equation relaxation kernel#1878sbryngelson wants to merge 1 commit into
sbryngelson wants to merge 1 commit into
Conversation
… kernel NVHPC's two-pass IPO (-Mextract/-Minline) miscompiles m_pressure_relaxation in OpenACC device code: inlining the equation-of-state chain into the six-equation relaxation kernel drops the internal-energy update entirely. It emits no diagnostic -- the build succeeds and the answer is wrong. The result is a conserved state where alpha and alpha_rho are correct and int_en is not. The smallest reproducer is one fluid, plain stiffened gas, model_eqns=3 (test 2F35A1FE), where internal energy comes out as exactly 0.0 instead of 2.5 while every other conserved variable matches its golden value. This has failed 23 model_eqns=3 tests on the NVHPC OpenACC lane since #1811, and because the failure is inherited from master it landed on every PR branched after it -- including PRs that only touched a shell script or documentation, which is what made it look spurious. The lane split is what identifies the cause: OpenMP offload skips this IPO pass (MFCTargets.cmake:26) and passes 713/713 on the same commits, Frontier CCE is not NVHPC and passes, and a CPU build of the same source on the same machine and compiler passes. Rebuilding OpenACC with IPO disabled makes all six reproducible 1D failures pass, and the targeted exclusion here does the same while keeping full IPO on the hot-path files. This is the third workaround for this pass, after the -Minline except: list and the existing m_start_up/m_cbc exclusions, and the first where it produced a wrong answer rather than a compile error. Verified on A100 / NVHPC 25.11, which reproduces the Phoenix V100 / NVHPC 25.5 failure bit for bit (0.80999999999592 vs a golden 0.80999999998385). Six of six reproducers pass, and a full 1D sweep goes from six tolerance failures to zero with no new ones. Note: verification here is 1D. The remaining failures in CI are 2D and 3D (axisymmetric, capillary, viscous IBM, recovering_sphere) and want a Phoenix run to confirm. Claude-Session: https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg
This was referenced Sep 13, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Gate the exclusion to OpenACC so CPU builds retain inlining.
Pull request overview
This pull request prevents an NVHPC IPO miscompilation in the pressure-relaxation kernel by excluding it from cross-file inlining.
Changes:
- Adds
m_pressure_relaxationto the NVHPC no-inline exclusions. - Documents the affected internal-energy corruption.
File summaries
| File | Description |
|---|---|
cmake/MFCTargets.cmake |
Configures the targeted NVHPC IPO exclusion. |
Review details
Suppressed comments (2)
cmake/MFCTargets.cmake:77
NVHPC_USE_TWO_PASS_IPOis enabled for non-Unified NVHPC builds regardless of GPU backend (cmake/GPU.cmake:213-219), so this guard also affects the CPU simulation target. That makes the CPU build compile this hot-path module with-Mnoinlineeven though the reported miscompile is OpenACC-only, unnecessarily losing inlining; gate the new exclusion onMFC_OpenACC AND ARGS_OpenACC.
This issue also appears on line 73 of the same file.
foreach(_no_inline_file m_start_up m_cbc m_pressure_relaxation)
set_source_files_properties(
"${CMAKE_BINARY_DIR}/fypp/${ARGS_TARGET}/${_no_inline_file}.fpp.f90"
TARGET_DIRECTORY ${ARGS_TARGET}
PROPERTIES COMPILE_OPTIONS "-Mnoinline"
cmake/MFCTargets.cmake:73
- The verification described for this compiler workaround covers only the 1D reproducers, while the reported regression set also contains 2D and 3D cases. Since this changes compile flags for the shared simulation module and is intended to fix all 23 failures, please run the remaining failing cases on the Phoenix NVHPC/OpenACC lane before relying on the exclusion; a 1D pass does not validate those execution paths.
foreach(_no_inline_file m_start_up m_cbc m_pressure_relaxation)
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 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.
NVHPC's two-pass IPO (
-Mextract/-Minline) miscompilesm_pressure_relaxationin OpenACC device code. Inlining the equation-of-state chain into the six-equation relaxation kernel drops the internal-energy update. There is no diagnostic — the build succeeds and the answer is wrong.Symptom
Minimal reproducer: one fluid, plain stiffened gas (
eos=2,qv=0),model_eqns=3— test2F35A1FE.Correct at t=0, destroyed during time stepping, in cells that should be quiescent. Elsewhere the corruption is additive with the fractional digits preserved (JWL:
2.57916258853339→0.14916258853359, exactly −2.43), which is a dropped term rather than a perturbed one.Why PRs looked spuriously broken
This has failed 23
model_eqns=3tests on the NVHPC OpenACC lane since #1811, and because it lives on master every branch cut after it inherits the failures. PR #1865 changed one file — a CI shell script — and failedeos=jwl -> model_eqns=3. Eight unrelated branches fail the identical 23 tests. Docs and CI PRs are hit hardest because non-.fppchanges bypass coverage selection and run the whole suite.Identifying the cause
The lane split does it:
MFCTargets.cmake:26)Rebuilding OpenACC with IPO disabled makes all six reproducible 1D failures pass. The targeted exclusion here does the same while keeping full IPO on the hot-path files (
m_rhs,m_riemann_solvers,m_viscous,m_weno).Ruled out along the way: a race (results are bit-identical across different GPUs), runtime-sized automatic arrays (applied the existing AMD workaround to NVHPC — no change), stale goldens (CPU passes), and missing device data (
eoss,eos_coeffs,any_state_dependent_eos,eqn_idxare all correctlyGPU_UPDATEd).Verification
On A100 / NVHPC 25.11, which reproduces the Phoenix V100 / NVHPC 25.5 failure bit for bit (
0.80999999999592against a golden0.80999999998385):Verification is 1D only. The rest of the 23 are 2D and 3D (axisymmetric, capillary, viscous IBM,
recovering_sphere) and want a Phoenix run to confirm.Note for reviewers
Third workaround for this pass, after the
-Minline except:list and them_start_up/m_cbcexclusions — and the first where it produced a wrong answer instead of a compile error. Per-file exclusion works until it doesn't, and the pass still covers the hot-path physics files. Whether to keep patching per-file or gate IPO for OpenACC the way OpenMP already is deserves a separate look.https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg