Fix two restart-write defects around zsolars in radmod - #63
Open
Sohex wants to merge 1 commit into
Open
Conversation
radstop saves zsolars, declared real :: zsolars(2), with mpputgp. mpputgp declares its dummy p(kdim,klev), gathers into a local z(NUGP,klev) via mpgagp and writes all of z, so mpgagp moves NHOR elements per rank from an array that holds 2, and the restart record is NUGP doubles of which two are the solar constants and the rest is buffer. At T42 on 16 ranks that is 8192 written for 2, with each rank reading 510 elements past the end of zsolars. zsolars is a global pair rather than a distributed gridpoint field, so it does not belong in a gather at all. solarini already uses the correct idiom for it. Separately, solarini's nine put_restart_array calls run at initialisation, when no restart file is open on nwriunit. Fortran connects fort.34 and writes them there, so every run directory gains a stray file and none of the nine arrays reaches the restart. They are the write half of a round trip whose read half is commented out below (the get_restart_array block under nstarfile > 0), where solarini was made unconditional; these arrays are recomputed from the namelist at every start and are not restart state. Neither change affects the integration. Nothing reads zsolars back, and restarts are located by name and skipped whole by reseek, so a record changing size cannot dislodge another. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two restart-write defects in
radmod.f90, both found while chasing something else. Neither affects the integration.1.
radstopwrites 8190 elements of buffer for a 2-element arrayzsolarsis declaredreal :: zsolars(2)(radmod.f90:251), andradstopsaves it withmpputgp(mpimod.f90) declares its dummyp(kdim,klev), allocates a localz(NUGP,klev), gathers into it withmpgagp, and writes all ofz:mpgagpmovesNHORelements per rank. At T42 on 16 ranks each rank reads 512 elements out of an array holding 2, and the record that reaches the file isNUGP= 8192 doubles: two solar constants and 8190 elements of gather buffer.zsolarsis a global pair, not a distributed gridpoint field, so it does not belong in a gather.solarinialready uses the right idiom for this array, so the fix is to use it inradstoptoo.This is normally invisible because
-finit-real=zerois in the default flags and holds the buffer at zero. It stops being invisible under-flto, where the compiler may skip zeroing a local it can prove is never read: the restart then differs run to run from one binary on one input. That is how this was found. It matters for anyone hashing restarts to identify them.2.
solariniwrites nine restart records to a unit nothing has openednwriunitis unit 34, andsolarini's nineput_restart_arraycalls run at initialisation, before any restart file is open on it. Fortran connectsfort.34and writes them there, so every run directory gains a stray 432-byte file and none of the nine arrays reaches the restart.They are the write half of a round trip whose read half is already commented out below, in the
get_restart_arrayblock undernstarfile > 0, wheresolariniwas made unconditional in its place. These arrays are recomputed from the namelist at every start, so removing the orphaned writes matches the decision already made there.Why this is safe
zsolarsback; theget_restart_arrayfor it is part of the commented-out blockreseeklocates records by name and skips data records whole, so a record changing size cannot dislodge another, and old and new restarts remain mutually readableVerified
On a fork of this model at T42 L10 on 16 ranks, 600 steps from a common restart:
zsolarsrecord is 16 bytes where it was 65536, still carrying both solar constantsfort.34is no longer createdHappy to split this into two PRs if you would rather review them separately.