qbmm: hoist s_mom_inv's internal procedures to module scope

[sbryngelson]

Problem

The five procedures contained in s_mom_inv — s_coeff_selector, s_chyqmom, s_hyqmom,
f_quad, f_quad2D — are internal procedures called from device code. On CCE 21.0.2 every
QBMM test aborts at runtime:

OpenMP Execution Error: src/simulation/m_qbmm.fpp:984 -
  unsupported access to Fortran host-associated variable in GPU internal procedure

Line 984 is call s_hyqmom(myrho, up, M1) inside s_chyqmom. Both are internal procedures of
s_mom_inv, so s_chyqmom reaches its sibling through the host frame chain, which CCE 21
does not support on device. (cray_inline=True inlines s_hyqmom into s_chyqmom, which is
why the abort surfaces at the call site rather than at the callee.)

Why hoisting is safe

None of the five references anything from its host scope. Every name that appears to be
shared is shadowed by the procedure's own dummy or local:

name	where it actually comes from
wght, abscX, abscY, momin	dummies of s_chyqmom
pres, rho, c, coeff	dummies of s_coeff_selector
q, r, s, wght_in	dummies of f_quad
moms, M1, M3, myrho, up, i1	locals
sgm_eps, nmom, nnode	parameters (compile-time constants)
weight, R0, nb	module variables, use-associated either way

Containment was organizational, not functional. Module scope removes the construct entirely
rather than working around the compiler.

The change is move-only

Apart from the removed contains, all 922 lines are byte-identical after dedent:

$ diff <(git show master:src/simulation/m_qbmm.fpp | grep -v '^\s*$' | sed 's/^[[:space:]]*//' | sort) \
       <(git show HEAD:src/simulation/m_qbmm.fpp   | grep -v '^\s*$' | sed 's/^[[:space:]]*//' | sort)
172d171
< contains

No logic, no expressions, no declarations changed.

Verification

Frontier (MI250X), CCE 21.0.2 / ROCm 7.2.0, MPI, 6/6:

test	--gpu mp	--gpu acc
0501B3DA 1D Viscosity → Bubbles → QBMM → bubble_model=3 → cfl_adap_dt=T	pass	pass
6784C02E 1D Viscosity → Bubbles → QBMM	pass	pass
83291843 2D Viscosity → Bubbles → QBMM → bubble_model=3	pass	pass

Confirmed independently in a second session on two further QBMM tests (284E0EF5,
48B9D0C4). A full-suite run with this change shows zero unsupported access to Fortran host-associated variable aborts across 627 tests, on both backends.

./mfc.sh precheck passes (7/7).

This is a correctness fix worth taking regardless of compiler: calling a sibling internal
procedure from device code is not portable, and no other file in src/simulation or
src/common still does it.

Context: #1684 (moving Frontier off cce/19.0.0).

[Copilot]

Pull request overview

This PR resolves a CCE 21.0.x OpenMP offload runtime abort in QBMM by eliminating calls between sibling internal procedures from device code, hoisting the five procedures previously contained within s_mom_inv to module scope in m_qbmm.

Changes:

• Removed the internal contains block inside s_mom_inv and ended s_mom_inv before the helper procedures.
• Hoisted s_coeff_selector, s_chyqmom, s_hyqmom, f_quad, and f_quad2D to module scope while keeping their bodies unchanged.
• Preserved module encapsulation (module is private by default; only the existing public symbols remain exported).

[codecov]

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.13%. Comparing base (aaecc47) to head (a040421).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_qbmm.fpp	96.36%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
+ Coverage   60.49%   61.13%   +0.64%     
==========================================
  Files          84       83       -1     
  Lines       21189    21047     -142     
  Branches     3134     3107      -27     
==========================================
+ Hits        12819    12868      +49     
+ Misses       6303     6126     -177     
+ Partials     2067     2053      -14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sbryngelson]

Independent confirmation from a second worktree, and a note that this PR supersedes a partial fix I had locally.

Confirmed on Frontier, CCE 21.0.2 / ROCm 7.2.0, both offload models, on two further QBMM tests beyond the three above. Same abort before the change, clean after.

This PR is strictly more complete than what I had. I hoisted only s_chyqmom and s_hyqmom — the two named in the abort — and left s_coeff_selector, f_quad and f_quad2D contained, with the contains block still present. That passed the QBMM tests, which is exactly why it is the wrong fix to take: all three of the ones I left behind are device procedures and two are actively called from device code.

s_coeff_selector   $:GPU_ROUTINE(..., cray_inline=True)   1 call site
f_quad             $:GPU_ROUTINE(parallelism='[seq]')     4 call sites
f_quad2D           $:GPU_ROUTINE(parallelism='[seq]')     0 call sites

s_coeff_selector carries cray_inline=True, so it is likely being inlined into its caller and the host-frame access never materialises — which would explain why a partial hoist passes the tests while leaving the construct in place. That makes it latent rather than fixed: any change to inlining, or a compiler that declines to inline it, brings the abort back. Removing the contains entirely, as this PR does, eliminates the construct rather than relying on an optimisation to hide it.

So I am dropping my version in favour of this one rather than opening a competing PR.

Two things worth adding to the record here:

The cray_inline=True on s_coeff_selector also explains the odd line attribution in the original abort — the error surfaces at the call site rather than the callee, which sent me looking at the wrong procedure initially.

And no other file under src/simulation or src/common still calls a sibling internal procedure from device code, so this appears to close the class rather than one instance. Worth a lint_source.py check if the pattern is considered forbidden going forward, since the failure mode is a runtime abort on one compiler rather than anything a build catches.

Context: the same port produced #1689 (build predicate) and a set of CCE 21 compiler defects with standalone reproducers; this one is application-side and correct to fix regardless of compiler.

[sbryngelson]

Cross-reference, so this is easy to place:

This fix is also carried in #1694 (the full Frontier CCE 21.0.2 move) as commit
05b4db36. This PR is the standalone version — it is a portability fix that is correct on
any compiler, so it can merge independently of whether the toolchain move is taken.

If #1694 lands first, this becomes a no-op and should be closed. If this lands first, #1694
rebases cleanly.

Trail for context:

• Frontier module set silently pins CCE 19, and hipfort drifts from the loaded ROCm #1684 — port tracking issue: full status, measurements, and what was deliberately not done
• Frontier: move to CCE 21.0.2 / ROCm 7.0.2 #1694 — the proposed CCE 21.0.2 / ROCm 7.2.0 move (draft pending a confirming run)
• Compiler defects with standalone reproducers: https://github.com/sbryngelson/compiler-bugs under cce/ (filed with OLCF/HPE)

The defect this fixes was found independently by two sessions working the same port. Worth
noting one of them landed a partial hoist first — moving only the two procedures named in
the runtime abort — and it passed the QBMM tests, because cray_inline=True was inlining the
remaining procedures and hiding the host-frame access. That fix was withdrawn in favour of
this one. The lesson generalises: the tests agreeing with you is not evidence the mechanism
is right.

[sbryngelson]

Full-suite validation, with this change included: 627/627 on both offload models, zero failures.

FULL SUITE (mp)    627 passed   0 failed   rc=0
FULL SUITE (acc)   627 passed   0 failed   rc=0

Frontier, CCE 21.0.2 / ROCm 7.2.0, MPI. Zero unsupported access to Fortran host-associated variable aborts, which is what this PR addresses, and zero aborts of any other kind.

For context on what else was needed to get there, since this PR alone is not sufficient to build MFC on CCE 21 — the port needed eight fixes in total, four of them working around compiler defects:

!DIR$ NOINLINE <name> is not a valid named directive (ftn-790); the named form is INLINENEVER, and the directive was being dropped entirely on Cray GPU builds	MFC bug
IBM passed pb_in/mv_in as absent optional args; CCE 21 dereferences the null dope vector	MFC bug
this PR — QBMM internal procedures reached through the host frame chain	MFC bug
is_configured() treats a bare CMakeCache.txt as built-and-installed	MFC bug, #1689
CCE 21 silently discards a dynamically-indexed store into a 1-based private array	compiler
CCE 21 builds a flat pointer from a private offset without the aperture — an object at frame offset 0 stores 4 GiB out of bounds	compiler
any defaultmap clause makes a device-resident array read as all zeros inside the target region	compiler
back-end assertion in AMDGPU Rewrite AGPR-Copy-MFMA blocks the device link outright	compiler

The defaultmap one was the largest single effect — MFC emitted those clauses on every OpenMP target loop as its translation of OpenACC default(present), so the immersed-boundary marker array read as empty and every IBM case failed. Removing them took the OpenMP suite from 566/627 to 622/627 in one change.

Performance, same source with the full fix set on both trees, six benchmark cases: CCE 21.0.2 is 5.8% faster than CCE 19.0.0 (328.27 s → 309.10 s total), faster in every case.

Reproducers for the compiler defects are in https://github.com/sbryngelson/compiler-bugs/tree/main/cce — eleven entries, six with standalone Fortran reproducers. Four are silent wrong-answer bugs rather than crashes, which is worth knowing for anyone else on CCE 21.

One note specific to this PR, from having written a narrower version of the same fix and thrown it away: hoisting only s_chyqmom and s_hyqmom — the two named in the abort — passes the QBMM tests, but leaves s_coeff_selector, f_quad and f_quad2D contained. All three are device procedures and two are called from device code; s_coeff_selector carries cray_inline=True, so it is almost certainly being inlined and the host-frame access simply never materialises. That partial fix passes because an optimisation is hiding the construct, and would break again on any change to inlining. Removing the contains outright, as this PR does, is the right shape.

[sbryngelson]

Merge order

This should merge before #1694. #1694 (the CCE 21 port) contains this same hoist, so landing this first means #1694 rebases with its 275-line move-only m_qbmm.fpp hunk dropping out — leaving a much smaller diff to review there, and keeping this change reviewable on its own terms.

Suggested order for the CCE 21 work:

order	PR	why
1	#1696 build predicate	no file overlap with anything; unblocks hand-testing the rest on Frontier
2	this PR	self-contained, correct on any compiler, no conflicts
3	#1694 CCE 21 port	rebase after this; also conflicts with #1679 — see the note there

This PR has no file overlap with #1696 or #1679, so it can go in whenever it is reviewed. It does not depend on the toolchain move: the construct it removes — calling a sibling internal procedure from device code — is non-portable regardless of which compiler is in use, and CCE 21 is simply the first to reject it outright.