Skip to content

Fix the NVHPC 26.9 link, and work around its any() miscompile #2111

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.

Sign up for GitHub

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

Draft
platelett wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix the NVHPC 26.9 link, and work around its any() miscompile #2111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e2e075
Work around an NVHPC 26.9 miscompile of any() over shear_indices
platelett Oct 26, 2026
5666e4f
Let -cudalib=nvtx3 pick the NVTX wrapper so NVHPC 26.9 links
platelett Oct 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/file-filter.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ python_src: &python_src

cmakelist: &cmakelist
- 'CMakeLists.txt'
- 'cmake/**'
Comment thread
platelett marked this conversation as resolved.

@sbryngelson sbryngelson Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is also in #2019; one of the two PRs should drop it. Related: ALWAYS_RUN_ALL_PREFIXES in toolchain/mfc/test/coverage.py lists toolchain/cmake/ but not cmake/. cmake/ changes still trigger the full run through the fallback rule, but adding the prefix there would keep the two lists in agreement.

@platelett platelett Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since #2019 was closed without merging, I've kept cmake/** here.

I've also added cmake/ to ALWAYS_RUN_ALL_PREFIXES, with a unit test for cmake/MFCTargets.cmake. As you noted, the fallback already selected the full suite, so this makes the rule explicit rather than changing test selection. The description now states that distinction.

- 'toolchain/cmake/**'

tests: &tests
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ jobs:
- { nvhpc: '26.1', target: gpu }
- { nvhpc: '26.3', target: cpu }
- { nvhpc: '26.3', target: gpu }
- { nvhpc: '26.9', target: cpu }
- { nvhpc: '26.9', target: gpu }

@sbryngelson sbryngelson Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes on this job:

  1. It only builds (--dry-run), so nothing in CI runs tests on 26.9. A fix or regression of the hypoelasticity miscompile would go unnoticed. A follow-up 26.9 job that runs just the hypoelastic tests (allowed to fail) would cover that.
  2. The matrix has continue-on-error: true, so this job cannot fail the workflow. It will only catch a link regression if someone reads its log. That policy predates this PR, so not a blocker, but worth knowing.

Also, the 26.9 testing in the description used the tarball, not nvcr.io/nvidia/nvhpc:26.9-devel-cuda_multi-ubuntu22.04. The job is still pending; let's confirm the image exists and fits on the runner before merging.

@platelett platelett Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • A 26.9 test job: added as a regular { nvhpc: '26.9', target: cpu } entry. I read your suggestion as a job that runs the tests, which on the GitHub-hosted runners means the CPU target. With the workaround in, it is an ordinary matrix entry that runs the full suite like the other releases, not a separate hypoelasticity-only job. On 26.9 with that job's flags (--test-all --no-mpi, FFLAGS=-tp=px -Kieee -noswitcherror), the hypoelasticity tests give 61 passed, 0 failed. The full suite without MPI passed earlier on 26.9 (714 passed, 0 failed).

  • The GPU job's steps on 26.9: the case-optimized 3D_sphbubcollapse build passes. If you meant running the tests on a GPU in CI: the self-hosted Phoenix NVHPC jobs use 25.5 (toolchain/modules), so that would need 26.9 there. Until then, the H100 runs above cover the workaround on a device.

  • continue-on-error: agreed, left as is.


fail-fast: false
continue-on-error: true
Expand Down
4 changes: 3 additions & 1 deletion cmake/MFCTargets.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,10 @@ exit 0
if (TARGET CUDA::nvToolsExt) # CUDA <= 12.8

@sbryngelson sbryngelson Oct 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is chosen by what FindCUDAToolkit finds (TARGET CUDA::nvToolsExt), not by the CUDA version nvfortran actually links against. On the NVHPC cuda_multi images (CI, docker.yml), CMake can find an older CUDA that still ships libnvToolsExt while nvfortran defaults to CUDA >= 12.9. Then the build links a mismatched NVTX library and skips -cudalib=nvtx3. Would it be simpler to always pass -cudalib=nvtx3 for NVHPC and let the driver choose?

@platelett platelett Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked whether the branch could be removed. NVHPC 21.7, 21.11, 22.2 and 22.3 reject -cudalib=nvtx3. Versions 22.11, 23.11 and 24.5 accept it but leave the NVTX calls unresolved when it is used alone. Those releases still need the CUDA::nvToolsExt path.

I also forced all four pairings of CMake's CUDA and nvfortran's CUDA on each of two releases: 25.5 with CUDA 11.8/12.9, and 26.9 with CUDA 12.9/13.3. With this PR, the NVTX test calls link and run in all eight combinations.

In the case you described on 25.5—CMake finding 11.8 while nvfortran uses 12.9—the build links 11.8's libnvToolsExt, which resolves the calls. That branch is unchanged. The reverse pairing also works with -cudalib=nvtx3 alone. On 26.9, all four pairings use the changed branch and pass; master fails all four because it explicitly requests the old wrapper name.

These are checks of the NVTX calls, rather than full CFD regression runs for every pairing.

target_link_libraries(${a_target} PRIVATE CUDA::nvToolsExt)
else() # CUDA >= 12.9
target_link_libraries(${a_target} PRIVATE nvhpcwrapnvtx)
# -cudalib=nvtx3 lets the NVHPC driver link its NVTX wrapper, whose name changes between releases.
# Only nvfortran understands the flag, so it must drive the link.
target_link_options(${a_target} PRIVATE "-cudalib=nvtx3")

@sbryngelson sbryngelson Oct 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my main concern. With the explicit nvhpcwrapnvtx removed, -cudalib=nvtx3 is now the only thing that puts the NVTX wrapper on the link line. The description says this was checked with -dryrun on 25.9, 26.3 and 26.9. Other releases also reach this branch (CUDA >= 12.9), for example 25.5, 25.7, 25.11 and 26.1, which CI builds. If any of them accepts -cudalib=nvtx3 but does not add the wrapper, nvtxRangePushA / nvtxRangePushEx (bound in src/common/m_nvtx.f90) end up unresolved and every GPU target fails to link. The old explicit library used to cover that case.

Could you run the same -dryrun check on the other releases in the CI matrix, or confirm that the GPU CI jobs for those versions actually link and not only configure?

Related: -cudalib only means something to nvfortran. If one of these targets is ever linked by a different compiler driver (for example after C/C++ sources are added), the flag is ignored and nothing else provides the NVTX symbols. Setting LINKER_LANGUAGE Fortran on these targets would guarantee the flag takes effect.

@platelett platelett Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Compatibility with the other NVHPC releases

Checked on 25.5, 25.7, 25.9, 25.11, 26.1, 26.3 and 26.9. MFC's actual m_nvtx.f90, with calls to nvtxRangePushA, nvtxRangePushEx and nvtxRangePop, links and runs with only -cudalib=nvtx3 under both -acc and -mp=gpu.

Using the environment settings from the CI images, FindCUDAToolkit finds CUDA 12.9 or newer and reports no CUDA::nvToolsExt, so these configurations exercise the changed branch.

For MFC itself, syscheck and pre_process build with both --gpu acc and --gpu mp on 25.5, 25.7, 25.11 and 26.1. All four targets build with both on 26.3 and 26.9. The release-by-release results are in the description.

  1. Fortran link driver

Added LINKER_LANGUAGE Fortran to the targets in the -cudalib=nvtx3 branch, so nvfortran continues to drive the link if C or C++ sources are added later.

set_target_properties(${a_target} PROPERTIES LINKER_LANGUAGE Fortran)

@sbryngelson sbryngelson Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this lands, NVHPC 26.9 builds succeed, but per the PR description 26.9 at -O2 miscompiles any(i == shear_indices) in m_variables_conversion.fpp and the hypoelasticity tests fail. Today users get a loud link error. After this, they get wrong physics with no diagnostic. Could we add a message(WARNING ...) (or FATAL_ERROR) when CMAKE_Fortran_COMPILER_VERSION is 26.9, or land the code workaround together with this?

@platelett platelett Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've included the workaround in this PR as a separate commit, before the link fix.

IS_SHEAR(idx) replaces all five any(... == shear_indices) checks with three explicit comparisons.

With the workaround, the hypoelasticity tests pass on the current code with NVHPC 26.9: 61 of 61 on CPU with the CI job's flags, and 62 of 62 on an H100 with both --gpu acc and --gpu mp, with MPI enabled.

Without the workaround, 13 of the first 23 tests fail on the H100 in each GPU mode before the run aborts. Failures were also reproduced on CPU.

The compiler bug and a reproducer are documented in the description.

endif()
endif()
endforeach()
Expand Down
6 changes: 6 additions & 0 deletions src/common/include/macros.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@
#endif
#:enddef

! Whether stress index idx is a shear component. Spelled out because NVHPC 26.9 at -O2 vectorizes any(idx == shear_indices)
! inside a loop to always true, which doubled the normal-stress elastic energy.
#:def IS_SHEAR(idx)
(${idx}$ == shear_indices(1) .or. ${idx}$ == shear_indices(2) .or. ${idx}$ == shear_indices(3))
#:enddef

#:def PROHIBIT(condition, message = None)
if (${condition}$) then
call s_prohibit_abort("${condition}$", ${message or '""'}$)
Expand Down
7 changes: 3 additions & 4 deletions src/common/m_variables_conversion.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,7 @@ contains
$:GPU_LOOP(parallelism='[seq]')
do i = eqn_idx%stress%beg, eqn_idx%stress%end
qK_prim_vf(eqn_idx%E)%sf(j, k, l) = qK_prim_vf(eqn_idx%E)%sf(j, k, &
& l) - f_elastic_energy(real(qK_prim_vf(i)%sf(j, k, l), wp), G_K, &
& any(i == shear_indices))/gamma_K
& l) - f_elastic_energy(real(qK_prim_vf(i)%sf(j, k, l), wp), G_K, ${IS_SHEAR('i')}$)/gamma_K
end do
end if

Expand Down Expand Up @@ -905,7 +904,7 @@ contains
q_cons_vf(eqn_idx%E)%sf(j, k, l) = q_cons_vf(eqn_idx%E)%sf(j, k, l) + (q_prim_vf(i)%sf(j, k, &
& l)**2._wp)/max(4._wp*G, verysmall)
! Double for shear stresses
if (any(i == shear_indices)) then
if (${IS_SHEAR('i')}$) then
q_cons_vf(eqn_idx%E)%sf(j, k, l) = q_cons_vf(eqn_idx%E)%sf(j, k, l) + (q_prim_vf(i)%sf(j, k, &
& l)**2._wp)/max(4._wp*G, verysmall)
end if
Expand Down Expand Up @@ -1242,7 +1241,7 @@ contains

E_e = 0._wp
do s = eqn_idx%stress%beg, eqn_idx%stress%end
E_e = E_e + f_elastic_energy(real(q_cons_vf(s)%sf(j, k, l), wp)/rho, G, any(s == shear_indices))
E_e = E_e + f_elastic_energy(real(q_cons_vf(s)%sf(j, k, l), wp)/rho, G, ${IS_SHEAR('s')}$)
end do

end function f_hypoelastic_energy
Expand Down
4 changes: 2 additions & 2 deletions src/simulation/m_riemann_state.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,14 +1280,14 @@ contains
if (G_L > verysmall) then
E_L = E_L + (tau_e_L(i)*tau_e_L(i))/(4._wp*G_L)
! Double for shear stresses
if (any(eqn_idx%stress%beg - 1 + i == shear_indices)) then
if (${IS_SHEAR('eqn_idx%stress%beg - 1 + i')}$) then
E_L = E_L + (tau_e_L(i)*tau_e_L(i))/(4._wp*G_L)
end if
end if
if (G_R > verysmall) then
E_R = E_R + (tau_e_R(i)*tau_e_R(i))/(4._wp*G_R)
! Double for shear stresses
if (any(eqn_idx%stress%beg - 1 + i == shear_indices)) then
if (${IS_SHEAR('eqn_idx%stress%beg - 1 + i')}$) then
E_R = E_R + (tau_e_R(i)*tau_e_R(i))/(4._wp*G_R)
end if
end if
Expand Down
1 change: 1 addition & 0 deletions toolchain/mfc/test/coverage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def save_map(path: Path, entries: dict, *, n_tests: int, git_sha: str, gfortran_
)
ALWAYS_RUN_ALL_PREFIXES = (
"src/common/include/", # GPU/Fypp macro & include files (CPU map can't line-attribute)
"cmake/", # build system
"toolchain/cmake/", # build system
"toolchain/bootstrap/", # build/run scripts
)
Expand Down
1 change: 1 addition & 0 deletions toolchain/mfc/test/test_coverage_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def test_macro_file_forces_all():
def test_cmake_forces_all():
assert is_always_run_all({"CMakeLists.txt"})
assert is_always_run_all({"toolchain/cmake/foo.cmake"})
assert is_always_run_all({"cmake/MFCTargets.cmake"})


def test_param_codegen_forces_all():
Expand Down
Loading