Skip to content

Make --mixed build again, and fix its multi-rank parallel I/O - #2106

Closed
ruru25254649 wants to merge 3 commits into
MFlowCode:masterfrom
ruru25254649:fix-mixed-precision
Closed

ruru25254649 wants to merge 3 commits into
MFlowCode:masterfrom
ruru25254649:fix-mixed-precision

Conversation

@ruru25254649

Copy link
Copy Markdown
Contributor

Summary

./mfc.sh build --mixed (half-precision storage stp, single-precision working wp) does not compile on master with NVHPC. Once it does, a run on two or more ranks with parallel_io = 'T' writes corrupted initial-condition and restart files. This PR fixes both. Non-mixed builds are unaffected, because there stp == wp, mpi_io_p == mpi_p and mpi_io_type == 1.

1. The build (commit 1)

On master 961cde5, ./mfc.sh build --mixed --no-gpu with NVHPC 26.3 fails in simulation:

NVFORTRAN-S-0155-Illegal POINTER assignment - type mismatch           (m_igr.fpp)
NVFORTRAN-S-0450-Argument number 1 to f_compute_shear_rate_from_components: kind mismatch   (m_viscous.fpp)

Each error has a single cause, and each fix is small:

  • real(stp) field elements passed to real(wp) dummies. This happens in m_viscous, m_bubbles_EE, m_acoustic_src and post_process/m_start_up (s_phase_temperature). The fix converts them at the call site with real(..., kind=wp). The dummies are intent(in), so nothing is written back.

  • jac in m_igr is declared real(wp). It is exposed through a scalar_field pointer, and scalar_field is real(stp). Every assignment to jac already converts to stp (jac(j, k, l) = real(..., kind=stp)), so it is now declared real(stp). jac_rhs and jac_old stay wp.

  • integer_field drops to integer(kind=1) under --mixed.

    • s_decode_patch_periodicity takes a default integer, which is the error left in m_ibm.fpp once the real-kind fixes are in.
    • The IB marker MPI-IO view is built from MPI_INTEGER.
    • ib_markers is updated with GPU_ATOMIC max.
    • An encoded periodic IB id (gbl_id + (num_gbl_ibs+1)*(xp + 3*yp + 9*zp)) exceeds 127 with only a few bodies.

    integer_field therefore stays a default integer in every mode. In mixed mode this costs 3 extra bytes per cell for ib_markers and bc_type. The one-byte kind was introduced in d290396.

2. Parallel I/O (commit 2)

Under --mixed a stored cell is 2 bytes (mpi_io_p = MPI_BYTE, mpi_io_type = 2). The field file views did not follow it:

  • s_initialize_mpi_data and s_initialize_mpi_data_ds built each field's subarray from mpi_p (4 bytes).
  • The MPI_FILE_SET_VIEW calls in pre_process/m_start_up, simulation/m_data_output and post_process/m_data_input used mpi_p as the etype, while the transfer right after uses mpi_io_p. Common Pitfalls in contributing.md states the rule: "mpi_io_p must match stp".
  • pre_process/m_start_up read data_size elements of mpi_p into a buffer of data_size 2-byte cells.

With one rank the layout still comes out right by accident. With two or more, each rank's block lands at the wrong offset.

The fix:

  • Subarrays are now built from a 2-byte contiguous cell type (s_create_mpi_field_subarray, a private helper in m_mpi_common).
  • Every view uses mpi_io_p as its etype.
  • The pre_process reader reads data_size*mpi_io_type elements of mpi_io_p.

The remaining mpi_p views (Lagrange bubble and post_process output) describe real(wp) buffers and are correct as they are.

Verification

All runs below are on an Intel Core i7-14700F (28 threads) and an RTX 4060 Ti (sm_89), using NVHPC 26.3 with its bundled HPC-X MPI.

Build:

Build master commit 1 commits 1+2
--mixed --no-gpu, pre/sim/post simulation fails (above) builds builds
--mixed --gpu acc, pre/sim/post simulation fails (also f_bulk_modulus, f_advance_step kind mismatches) builds builds

End to end:

  • Case: examples/2D_riemann_test at 64², 20 steps, parallel_io = 'T', --mixed.
  • Comparison: byte-for-byte, the 1-rank and 2-rank restart_data/lustre_<step>.dat files (40,960 bytes each = 64·64·5 fields·2 bytes).
lustre_0 (initial condition) lustre_10 lustre_20
commit 1 only: 1 rank vs 2 ranks differ: 3,272 of 20,480 values; density off by up to 0.97 (range 0.14–1.5) differ differ
commits 1+2: 1 rank vs 2 ranks identical identical identical
1-rank file, commit 1 vs 1+2 identical identical identical
commits 1+2 on GPU (--gpu acc, 4060 Ti): 1 rank vs 2 ranks identical — identical

Against a double-precision run of the same case (GNU, 1 rank), density at step 20:

--mixed, 2 ranks max relative difference from double
commits 1+2, CPU 1.1e-3 (half-precision storage)
commits 1+2, GPU 1.1e-3
commit 1 only, CPU 6.5e-1

toolchain/mfc/test_mixed_mpi_views.py:

  • What it does: compiles the production s_initialize_mpi_data and helper, plus every production field SET_VIEW/transfer pair, against two-byte opaque cells. It needs no REAL(2) support. It writes from 2 ranks and compares the file with a serially written reference.
  • Results: fails on master; passes with this PR (Open MPI 4.1.6 + gfortran 13.3, and HPC-X + nvfortran 26.3). It skips without an MPI compiler, and under MFC_SKIP_COMPILER_TESTS=1, like the other compiler tests.

Non-mixed builds:

  • Full regression suite, GNU 13.3 + Open MPI 4.1.6, --no-mixed, CPU (on a branch merging all candidates): 725 passed, 0 failed, 39 convergence cases skipped by default. No golden changes.
  • ./mfc.sh lint (CI lint gate, including the compiler tests): passes.

Not covered: Cray and AMD flang with --mixed. Cray's CPU --mixed failure in #1965 (kind 2 not valid for REAL) happens earlier, in m_derived_types, and is not addressed here.

Side effects

  • Non-mixed builds: no change. stp == wp, mpi_io_p == mpi_p and mpi_io_type == 1 there, so the conversions are no-ops and the new helper builds exactly the old subarray. Full suite: 725 passed.
  • --mixed builds:
    • integer_field (ib_markers, bc_type) is 4 bytes instead of 1. That costs 3 extra bytes per cell for ib_markers, and serialized IB-marker files from an old --mixed build cannot be read back; regenerate them. No such build could be produced on master with NVHPC, because it did not compile.
    • jac (IGR) is now stored in stp, which is what every assignment to it already produced.
  • Overlap with SCC 26: bug: fix parallel I/O failures and performance overhead #2085 (%sf → %sf(:,:,:) in the same MPI-IO calls): the two PRs conflict textually in pre_process/m_start_up, simulation/m_data_output and post_process/m_data_input. The resolution keeps both changes (mpi_io_p etype and %sf(:,:,:)). Merged that way, the new view test passes, and on SCC 26: bug: fix parallel I/O failures and performance overhead #2085 alone it fails with 774 mismatched bytes. The test accepts either spelling, so whichever PR lands second only needs that one-line-per-call rebase.

Contribution Policy

All contributions are expected to demonstrate:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.

Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

This PR is from Your Team Name participating in the SC26 Student Cluster Competition.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC

ruru25254649 and others added 2 commits October 25, 2026 03:52
A --mixed build (stp = half, wp = single) no longer compiles with
nvfortran:

- m_viscous, m_bubbles_EE, m_acoustic_src and post_process m_start_up
  pass real(stp) field elements to procedures whose dummies are
  real(wp). Convert them with real(..., kind=wp) at the call site.
- m_igr exposes jac through a scalar_field pointer, which is real(stp),
  while jac was declared real(wp). Every write to jac already converts
  to stp, so declare it real(stp) to match.
- integer_field dropped to integer(kind=1) under --mixed, but
  s_decode_patch_periodicity and the MPI_INTEGER I/O expect default
  integers, and encoded periodic IB ids exceed 127. Keep it default.

Non-mixed builds are unchanged: stp == wp there, so the conversions
are no-ops and integer_field was already a default integer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC
Under --mixed each stored cell is 2 bytes (mpi_io_p = MPI_BYTE,
mpi_io_type = 2), but the field subarray views were built from mpi_p
(4 bytes) and several file views used mpi_p as their etype. With two
or more ranks the ranks' blocks land at the wrong offsets, so restart
and initial-condition files are corrupted.

Build the subarray from a 2-byte contiguous cell type, use mpi_io_p as
the etype everywhere the transfer uses mpi_io_p, and read
data_size*mpi_io_type elements in pre_process. Non-mixed builds are
unchanged: mpi_io_p == mpi_p and mpi_io_type == 1 there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC
@ruru25254649
ruru25254649 requested a review from a team as a code owner October 25, 2026 07:04
@ruru25254649
ruru25254649 requested review from wilfonba and removed request for a team October 25, 2026 07:04
@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: 2a8dc39

Files changed:

  • 11
  • src/common/m_derived_types.fpp
  • src/common/m_mpi_common.fpp
  • src/post_process/m_data_input.f90
  • src/post_process/m_start_up.fpp
  • src/pre_process/m_start_up.fpp
  • src/simulation/m_acoustic_src.fpp
  • src/simulation/m_bubbles_EE.fpp
  • src/simulation/m_data_output.fpp
  • src/simulation/m_igr.fpp
  • src/simulation/m_viscous.fpp

Findings:

  • medium src/simulation/m_viscous.fpp: The off-diagonal shear-rate arguments, 0.5_wp*(grad_y_vf(1)%sf + grad_x_vf(2)%sf) and the z-direction analogues, are still computed from raw stp operands. The diagonal arguments were wrapped in real(..., kind=wp). In mixed precision the sum of two stp values is evaluated at stp precision before it is promoted, so the off-diagonal terms lose precision and can overflow in half. Wrap each operand in real(..., kind=wp) for consistency. Moderately sure; this only matters when stp is lower precision than wp.
  • medium src/simulation/m_igr.fpp: jac is now real(stp) but jac_rhs and jac_old stay real(wp). The rest of m_igr.fpp is not in the context files, so I could not check every use of jac. Any pass of jac to a routine with real(wp) dummies, or any GPU copy between jac and jac_old, would mismatch kinds under mixed precision. Check all jac uses, including the UVM jac_host pointer association. Low confidence.

@ruru25254649
ruru25254649 marked this pull request as draft October 25, 2026 07:18
The off-diagonal shear-rate arguments in m_viscous and the phase
temperature density in post_process combined two real(stp) values and
converted only the result, so under --mixed the sum, quotient and max
ran in half precision (max also mixed stp with the wp sgm_eps). Each
operand is now converted first. No-op when stp == wp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC
@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/common/m_mpi_common.fpp 1503 +9
src/common/m_derived_types.fpp 479 -4
src/simulation/m_viscous.fpp 1023 +2
src/simulation/m_igr.fpp 2290 +1
Directory Lines Diff
common 10535 +5
simulation 28566 +3
total 47634 +8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant