Refactor /common and beta buffer routines for future particle inclusion - #1672
Refactor /common and beta buffer routines for future particle inclusion#1672joshgillis wants to merge 14 commits into
Conversation
sbryngelson
added
claude-full-review
|
Claude Code Review Head SHA: df0a969 Files changed:
Findings:
|
|
I'm not sure what the review means about the updated call site for s_beta_extrapolation, I'm actually not even sure if where that subroutine is called. I can remove the changes there if that subroutine is not in use. As for the PR as a whole, if it is preferred that this structure is not added until particles are functional I can wait and make other adjustments. |
|
I believe |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1672 +/- ##
==========================================
+ Coverage 61.04% 61.08% +0.03%
==========================================
Files 83 83
Lines 20978 20975 -3
Branches 3099 3098 -1
==========================================
+ Hits 12807 12813 +6
+ Misses 6126 6114 -12
- Partials 2045 2048 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Following up on my approval with some things I want answered before this lands. The mechanical part of the refactor checks out: v_size = nVar is reassigned locally at the top of s_mpi_reduce_beta_variables_buffers, so the pack loop bounds stay consistent with vars_comm; mapCells is a parameter in m_constants.fpp, so the new sizing block has no init-order hazard; the max() lands before the @:ALLOCATE; and the s_beta_extrapolation removal is clean (no references left anywhere in src/).
My concerns are about coverage and about the halo-size change being more than a refactor. Two of the inline comments are pre-existing issues rather than things this PR introduces — I have marked those, and I am fine with them going in a separate PR, but this change rewrites every line involved so it seemed worth raising now.
Leaving this as a comment rather than re-requesting changes; the approval stands unless the answers to items 1-3 turn up something.
| !! @param mpi_dir MPI communication coordinate direction | ||
| !! @param pbc_loc Processor boundary condition (PBC) location | ||
| subroutine s_mpi_reduce_beta_variables_buffers(q_comm, kahan_comp, mpi_dir, pbc_loc, nVar) | ||
| subroutine s_mpi_reduce_beta_variables_buffers(q_comm, kahan_comp, mpi_dir, pbc_loc, nVar, vars_comm) |
There was a problem hiding this comment.
This routine has no CI coverage. alter_lag_bubbles in toolchain/mfc/test/cases.py sets no ppn, so every Lagrange golden runs on a single rank. The call site in s_populate_beta_bc_direction is guarded by if (bc_bounds%beg >= 0 .or. bc_bounds%end >= 0), which requires an actual MPI neighbor, so nothing in the suite enters this routine at all.
That means roughly 120 changed lines here, plus the new halo sizing, are exercised by no test. Was the Tuolumne run multi-rank? If not, this refactor is currently unverified end to end.
Please add a ppn=2 Lagrange golden. It is the only thing that would actually cover the pack/unpack rewrite.
There was a problem hiding this comment.
After making the other changes I will add in the ppn=2 test and verify the multi-rank results. I see in a few of the test cases that cases.append(define_case_d(stack, "", {}, ppn=2)) is added to the end to create a multi-rank test, does adding that to the end of alter_lag_bubbles produce a second test with mpi?
| else | ||
| beta_halo_size = 2*(mapCells + 1)*beta_v_size - 1 | ||
| end if | ||
| halo_size = max(halo_size, beta_halo_size) |
There was a problem hiding this comment.
This is a behavior change, not a refactor. Before this PR the beta exchange reused a buff_send/buff_recv sized only from buff_size*v_size*(...). The beta path needs 2*(mapCells + 1)*3*max(...), i.e. 24*max(...) with mapCells = 3. For 2D Lagrange cases with small sys_size that exceeds the conservative-variable sizing, which means the old code was overrunning the send buffer.
If that reading is right, this is a latent buffer overflow fix arriving under "Type of change: Refactor" with the tests checkbox unchecked and the commit message "Adding integers for halo expansion". Please call it out explicitly in the PR description, and say which configurations actually overflowed before — that determines whether this needs to be backported.
If my reading is wrong and the old sizing was always sufficient, I would like to understand why this block is needed at all.
There was a problem hiding this comment.
This change was made in preparation for overflows from the additional particle variables that were reported by Jose, but is preemptive since this version has not been tested with particles, and improperly included given this PR's main focus. I will remove this and note it potentially for later.
| end if | ||
|
|
||
| if (bubbles_lagrange) then | ||
| beta_v_size = size(beta_vars) |
There was a problem hiding this comment.
The generalization stops short of the buffer sizing. The call chain now takes a generic vars_comm, but the sizing here is still hard-wired to size(beta_vars) and the whole block is gated on if (bubbles_lagrange).
The stated purpose of the PR is that particle routines will pass vars_send instead. If size(vars_send) > 3, or if particles can be active without bubbles_lagrange, halo_size will not cover the particle exchange and you get exactly the overflow the block above is fixing.
Since this PR exists specifically to prepare for that, the sizing should be driven by whatever mapping is actually registered rather than by beta_vars. Otherwise the follow-up PR inherits a silent overflow that will be much harder to spot once particles are in the mix.
There was a problem hiding this comment.
Connected to the other halo change, this will be removed and I'll wait to add it in until a more appropriate time
| end if | ||
|
|
||
| $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2) | ||
| $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2, copyin = '[vars_comm]') |
There was a problem hiding this comment.
beta_vars is already device-resident — $:GPU_DECLARE(create='[comm_coords, comm_size, beta_vars]') at m_mpi_common.fpp:34 and $:GPU_UPDATE(device='[beta_vars]') at m_mpi_common.fpp:102. Routing it through an assumed-shape dummy and adding copyin here replaces a device-resident read with a host-to-device transfer on every launch of this loop: six launches per timestep (two locations times three directions) in a hot BC path.
The payload is three integers, so this is not about bandwidth — it is the per-launch transfer and the descriptor handling for an assumed-shape dummy. Did the Tuolumne runs show any cost here?
A device-resident module-level array, or passing an integer selector instead of the array, would avoid it entirely.
Minor style note while here: copyin = '[vars_comm]' has spaces around = where every other clause in the file is written copyin='[...]'.
There was a problem hiding this comment.
I'm considering introducing a device-resident communication mapping (comm_vars) in m_mpi_common. The bubble routines would register beta_vars with this mapping, while the particle routines would register vars_send. The communication routines would then operate on comm_vars, avoiding the per-kernel copyin while also allowing the halo sizing to be driven by the active communication mapping. Do you think that this would be an improvement?
| integer, intent(in) :: k, l | ||
| integer, intent(in) :: nvar | ||
| integer :: j, i | ||
| integer, dimension(:), intent(in) :: vars_comm |
There was a problem hiding this comment.
Pre-existing, but this PR rewrites every line that depends on it. q_beta and kahan_comp are declared dimension(num_dims + 1) just above — 3 in 2D, 4 in 3D. With nvar = 3 this routine indexes vars_comm(3), which is beta_vars(3) = 5. Indexing element 5 of a dummy declared with 3 or 4 elements is not conforming and is fatal under -fcheck=bounds.
The parent s_populate_beta_buffers already declares these as dimension(:). Please make these two match — it is a one-line change per declaration and it removes the discrepancy at the point where you are already touching every use. Same applies to s_beta_reflective below.
See my comment on the nvar = 3 call site in m_bubbles_EL.fpp for the underlying problem.
There was a problem hiding this comment.
I will make those changes
| call nvtxStartRange("BUBBLES-LAGRANGE-BETA-COMM") | ||
| if (lag_params%cluster_type >= 4) then | ||
| call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 3) | ||
| call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 3, beta_vars) |
There was a problem hiding this comment.
Pre-existing, flagging because this line is being touched. nvar = 3 means the comm routines reach beta_vars(3) = 5, so q_beta(5) must exist. It only does when q_beta_idx = 6, which requires lag_params%solver_approach == 2 .and. p == 0 (see lines 92-103).
For 3D two-way coupling q_beta_idx = 4, so q_beta(5) is written past the end of the allocation. Neither m_checker.fpp nor case_validator.py prevents cluster_type >= 4 from being combined with a 3D two-way setup, and s_deltafunc / s_smearfunction in m_bubbles_EL_kernels.fpp write updatedvar(5) under the same condition.
No example or test in the repo sets cluster_type >= 4, so this entire path is unexercised — which is presumably why it has survived.
Worth adding the constraint (cluster_type >= 4 requires solver_approach == 2 and p == 0), either here or in a follow-up.
There was a problem hiding this comment.
I will add this constraint
| !> Initialize the module. | ||
| impure subroutine s_initialize_mpi_common_module | ||
|
|
||
| integer :: beta_v_size, beta_comm_size_1, beta_comm_size_2, beta_comm_size_3, beta_halo_size |
There was a problem hiding this comment.
Minor: these are declared outside #ifdef MFC_MPI but only used inside it, so non-MPI builds will warn about five unused variables. Moving the declaration inside the guard fixes it.
There was a problem hiding this comment.
It looks like this line is causing the large group of failing checks, gpt is suggested to move this line before the newly added exchange_all_chemistry_temperatures line, but I don't think that would fix the non-mpi build warning. Regardless, I will probably remove this addition entirely until the explicit need for a halo increase is verified.
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Review of the current state plus some alternative designs worth weighing before this leaves draft. Two concrete bugs are inline; the design discussion is below.
The direction is right, and two things here are genuine improvements independent of everything else: the beta_halo_size addition is a real fix (beta comm reuses buff_send/buff_recv, which were previously sized only from v_size), and deleting s_beta_extrapolation is clean — it was already dead on master, defined but never called.
Alternative designs
The thing worth stepping back on: beta_vars = [1, 2, 5] exists only because the communicated slots of q_beta aren't contiguous. Threading a runtime index list down into device code is what forces the copyin, adds a dependent load (q_beta(vars_comm(i))) in the innermost loop, and creates the mapping bug flagged inline. Each option below either makes that indirection free or removes it.
A. Pass a scalar selector; keep the map device-resident
Smaller diff than the current approach, and it makes the mapping bug structurally impossible.
! m_mpi_common
integer, parameter :: COMM_MAP_BUBBLES = 1, COMM_MAP_PARTICLES = 2
integer :: comm_var_map(1:3, 1:2) = reshape([1,2,5, 1,2,3], [3,2])
$:GPU_DECLARE(create='[comm_var_map]')The call chain carries integer, intent(in) :: comm_map_id — a scalar, not an array — so no descriptor crosses into device code and no copyin is needed anywhere. vars_send becomes column 2 when particles land. Five signatures gain an integer instead of an assumed-shape array.
B. Just make the dummy explicit-shape
Smallest edit to what is already written:
integer, intent(in) :: nvar
integer, dimension(nvar), intent(in) :: vars_comm ! not dimension(:)No descriptor, and it self-documents that only nvar entries are read. You would still need copyin on every kernel, so A is strictly better — but this is a two-line change.
C. Delete the concept: reorder q_beta so communicated slots are contiguous
Then the map disappears entirely — do i = 1, nvar over q_beta(i), no extra argument in any signature, and the inner loop loses an indirection. Invasive: slot meanings are positional across roughly 40 sites in m_bubbles_EL.fpp, and q_beta_idx is 3/4/6 depending on solver_approach and dimensionality. This is the only option where the follow-up particle PR adds nothing to these signatures.
Worth pricing, but I would keep it as a separate PR — mixing a q_beta reindex into a comm refactor makes bisecting a golden-file regression miserable.
D. Build the map at init and assert the invariant
Independent of A–C. The call site currently encodes an unstated invariant:
call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 3, beta_vars) ! m_bubbles_EL.fpp:875
call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 2, beta_vars) ! :877Those bare 3/2 rely on beta_vars(3) = 5, and q_beta(5) only exists when q_beta_idx == 6, which requires solver_approach == 2 .and. p == 0. So cluster_type >= 4 must imply q_beta_idx == 6 — nothing checks that, and the q_beta(5) read at :748 sits inside exactly that branch. Deciding the count once at init, next to where q_beta_idx is set, with an @:ASSERT, removes both magic numbers and makes the coupling checkable.
Also worth folding in: v_size is shared mutable state
v_size is module-private (m_mpi_common.fpp:26) and both comm paths assign it — :593/:601/:605 for the regular halo exchange, :1128 for beta — each followed by a GPU_UPDATE. That works today only because the two paths never interleave. A refactor whose stated goal is reusing one comm path for bubbles and particles is exactly when that stops holding. Making it a local, or a component of a small comm-context type, is cheap now and painful later.
Suggested combination
A + D. Together that is a smaller diff than the current version, eliminates the GPU mapping bug rather than patching it, removes the magic numbers, and leaves particles as a one-column addition. C as a follow-up if you want it genuinely clean.
| exchange_all_chemistry_temperatures = exchange_all_chemistry_temperatures_in | ||
| use_rdma_transport = use_rdma_transport_in | ||
|
|
||
| integer :: beta_v_size, beta_comm_size_1, beta_comm_size_2, beta_comm_size_3, beta_halo_size |
There was a problem hiding this comment.
Compile blocker: this declaration follows two executable statements (the assignments to exchange_all_chemistry_temperatures and use_rdma_transport just above), which Fortran does not allow.
Confirmed with gfortran -fsyntax-only on a reduced case: Error: Unexpected data declaration statement.
Needs to move above line 56, with the other declarations.
Flagging because the PR description mentions the suite ran on CPU and Tuolumne — that can't apply to this commit as it stands, so I suspect the declaration got displaced in a rebase and the testing predates it. Worth re-running once this is fixed, since the result would also bear on the device-mapping issue below.
| & *(l - comm_coords(3)%beg))) | ||
| buff_send(r) = real(q_comm(beta_vars(i))%sf(j + pack_offset, k, l), & | ||
| & kind=wp) - real(kahan_comp(beta_vars(i))%sf(j + pack_offset, k, l), kind=wp) | ||
| buff_send(r) = real(q_comm(vars_comm(i))%sf(j + pack_offset, k, l), & |
There was a problem hiding this comment.
vars_comm is dereferenced on the device here without being mapped.
This kernel and five others in this routine (the three pack loops and three unpack loops) index q_comm(vars_comm(i)) inside GPU_PARALLEL_LOOP, but none of them has a data clause for vars_comm — the unpack loops copy in only replace_buff.
What this replaced was device-resident: beta_vars is module-level with GPU_DECLARE(create=...) at line 36 and GPU_UPDATE(device=...) at line 113. An assumed-shape dummy argument is not.
The reason I think this is a real bug rather than a misreading: the equivalent loop in m_boundary_common.fpp:551 did get copyin = '[vars_comm]' in this same PR. It is needed in both places or neither.
Note this would very likely pass on Tuolumne regardless — MI300A is a unified-memory APU, which is exactly the configuration that hides a missing map. The #ifndef __NVCOMPILER_GPU_UNIFIED_MEM paths in this file are where it would surface. Worth a discrete-GPU run before this leaves draft.
Design option A in the review body avoids this class of bug entirely by passing a scalar selector instead of an array.
| end if | ||
|
|
||
| $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2) | ||
| $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2, copyin = '[vars_comm]') |
There was a problem hiding this comment.
This is the copyin that is missing from the six equivalent kernels in s_mpi_reduce_beta_variables_buffers — see the inline note on m_mpi_common.fpp. Worth reconciling the two.
Minor, if you keep this approach: copyin on a per-launch basis in a routine that runs every timestep per direction adds a host-to-device transfer of a 3-element array each time. Negligible in absolute terms, but avoidable — a device-resident module table (option A) costs nothing per launch.
Also copyin = '[vars_comm]' has spaces around = where the other keyword arguments here don't.
| else | ||
| beta_halo_size = 2*(mapCells + 1)*beta_v_size - 1 | ||
| end if | ||
| halo_size = max(halo_size, beta_halo_size) |
There was a problem hiding this comment.
Minor: beta_halo_size is declared default integer, while halo_size is integer(kind=8) (line 42). The whole product on lines 91-97 is therefore evaluated in 32-bit and only widened at this assignment.
gfortran accepts the mixed-kind max as an extension, so this is not a compile error — but it does defeat the reason halo_size is 64-bit in the first place. Declaring beta_halo_size as integer(kind=8) costs nothing and keeps the arithmetic honest for large per-rank domains.
3 participants
Description
The beta buffer communication routines were generalized to support both bubble and particle data. Previously, these routines were hard-coded to use beta_vars, which defines the communication mapping for bubble variables. Particle communication will require a different mapping (vars_send). Instead of duplicating the communication routines for each case, a generic communication variable list (vars_comm) is now passed through the call chain. Bubble routines pass beta_vars, while particle routines will pass vars_send, allowing the same communication infrastructure to be reused for both implementations.
Type of change (delete unused ones)
Testing
How did you test your changes?
Test suite ran on personal machine cpu, and tuolumne gpu
Checklist
Check these like this
[x]to indicate which of the below applies.See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)AI code reviews
Reviews are not retriggered automatically. To request a review, comment on the PR:
@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label