Repository navigation
Make --mixed build again, and fix its multi-rank parallel I/O - #2106
Closed
ruru25254649 wants to merge 3 commits into
Closed
ruru25254649 wants to merge 3 commits into
ruru25254649 wants to merge 3 commits into
Conversation
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
|
Claude Code Review Head SHA: 2a8dc39 Files changed:
Findings:
|
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
Lines of Code
|
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.
Summary
./mfc.sh build --mixed(half-precision storagestp, single-precision workingwp) does not compile on master with NVHPC. Once it does, a run on two or more ranks withparallel_io = 'T'writes corrupted initial-condition and restart files. This PR fixes both. Non-mixed builds are unaffected, because therestp == wp,mpi_io_p == mpi_pandmpi_io_type == 1.1. The build (commit 1)
On master
961cde5,./mfc.sh build --mixed --no-gpuwith NVHPC 26.3 fails insimulation:Each error has a single cause, and each fix is small:
real(stp)field elements passed toreal(wp)dummies. This happens inm_viscous,m_bubbles_EE,m_acoustic_srcandpost_process/m_start_up(s_phase_temperature). The fix converts them at the call site withreal(..., kind=wp). The dummies areintent(in), so nothing is written back.jacinm_igris declaredreal(wp). It is exposed through ascalar_fieldpointer, andscalar_fieldisreal(stp). Every assignment tojacalready converts tostp(jac(j, k, l) = real(..., kind=stp)), so it is now declaredreal(stp).jac_rhsandjac_oldstaywp.integer_fielddrops tointeger(kind=1)under--mixed.s_decode_patch_periodicitytakes a default integer, which is the error left inm_ibm.fpponce the real-kind fixes are in.MPI_INTEGER.ib_markersis updated withGPU_ATOMICmax.gbl_id + (num_gbl_ibs+1)*(xp + 3*yp + 9*zp)) exceeds 127 with only a few bodies.integer_fieldtherefore stays a default integer in every mode. In mixed mode this costs 3 extra bytes per cell forib_markersandbc_type. The one-byte kind was introduced in d290396.2. Parallel I/O (commit 2)
Under
--mixeda 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_dataands_initialize_mpi_data_dsbuilt each field's subarray frommpi_p(4 bytes).MPI_FILE_SET_VIEWcalls inpre_process/m_start_up,simulation/m_data_outputandpost_process/m_data_inputusedmpi_pas the etype, while the transfer right after usesmpi_io_p. Common Pitfalls incontributing.mdstates the rule: "mpi_io_pmust matchstp".pre_process/m_start_upreaddata_sizeelements ofmpi_pinto a buffer ofdata_size2-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:
s_create_mpi_field_subarray, a private helper inm_mpi_common).mpi_io_pas its etype.pre_processreader readsdata_size*mpi_io_typeelements ofmpi_io_p.The remaining
mpi_pviews (Lagrange bubble andpost_processoutput) describereal(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:
--mixed --no-gpu, pre/sim/postsimulationfails (above)--mixed --gpu acc, pre/sim/postsimulationfails (alsof_bulk_modulus,f_advance_stepkind mismatches)End to end:
examples/2D_riemann_testat 64², 20 steps,parallel_io = 'T',--mixed.restart_data/lustre_<step>.datfiles (40,960 bytes each = 64·64·5 fields·2 bytes).lustre_0(initial condition)lustre_10lustre_20--gpu acc, 4060 Ti): 1 rank vs 2 ranksAgainst a double-precision run of the same case (GNU, 1 rank), density at step 20:
--mixed, 2 rankstoolchain/mfc/test_mixed_mpi_views.py:s_initialize_mpi_dataand helper, plus every production fieldSET_VIEW/transfer pair, against two-byte opaque cells. It needs noREAL(2)support. It writes from 2 ranks and compares the file with a serially written reference.MFC_SKIP_COMPILER_TESTS=1, like the other compiler tests.Non-mixed builds:
--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--mixedfailure in #1965 (kind 2 not valid for REAL) happens earlier, inm_derived_types, and is not addressed here.Side effects
stp == wp,mpi_io_p == mpi_pandmpi_io_type == 1there, so the conversions are no-ops and the new helper builds exactly the old subarray. Full suite: 725 passed.--mixedbuilds:integer_field(ib_markers,bc_type) is 4 bytes instead of 1. That costs 3 extra bytes per cell forib_markers, and serialized IB-marker files from an old--mixedbuild 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 instp, which is what every assignment to it already produced.%sf→%sf(:,:,:)in the same MPI-IO calls): the two PRs conflict textually inpre_process/m_start_up,simulation/m_data_outputandpost_process/m_data_input. The resolution keeps both changes (mpi_io_petype 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:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
This PR is from Your Team Name participating in the SC26 Student Cluster Competition.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC