Replace three enclosing-scope fypp macros with explicit procedures - #9
Merged
sbryngelson merged 3 commits intoAug 28, 2026
Conversation
Both expanded into the caller's scope, so a call site said nothing about what they read or wrote. Neither needed to. hll_flux_component took two arguments and pulled S_L, S_R, F_L, F_R, U_L and U_R from whatever routine expanded it. Its own comment claimed codegen identical to the materialized expression; f_hll_flux makes that checkable instead of asserted, and the offload image confirms it. compute_capillary_stress_tensor took none at all, reading sigma, w1, w2, w3, normW and writing Omega implicitly. It was the only content of include/inline_capillary.fpp, which goes with it. Omega is intent(inout), not intent(out): the entries a given dimensionality does not define are left as the caller had them, which is what the macro did. Making them intent(out) would leave them undefined in 1D and 2D. 0 regression(s) across 471 kernels. Hypoelasticity 59, Viscous 63, and the four capillary and surface_tension cases - 126 passed, 0 failed. Part of MFlowCode#1769.
compute_axis_inv_re expanded into s_compute_viscous_stress_cylindrical_boundary four times, reading grad_x_vf, grad_y_vf, grad_z_vf, alpha_visc, j, k, l and writing gamma_dot_c and Re_visc, none of it visible at the call site. gamma_dot_c was only ever live inside the macro, so it becomes a local of the new routine and leaves the caller's declarations and all four private() clauses - one less variable of per-thread state in each of those loops. This is the signature MFlowCode#1714 warned about: three scalar_field array dummies on a [seq] device routine. It does not reproduce here. The distinction holds - MFlowCode#1714 constructed a derived type and took its address, while these arrays already exist and are passed by reference. viscous_weno5_sgb_acoustic, MI210, amdflang, three runs each: before 3.4571 3.4446 3.4396 (mean 3.4471) after 3.4272 3.4383 3.4235 (mean 3.4297) -0.50%, every after run below every before run, consistent with the smaller private clauses. Viscous 63 passed, 0 failed. Static resources move by one kernel: m_riemann_solver_hlld gains 16 B of scratch on 21440, in a module this change cannot reach. That is the whole-image codegen variance of MFlowCode#1759, not a consequence of the refactor; every viscous kernel is unchanged. Part of MFlowCode#1769.
compute_elastic_wave_speeds_lr wrote s_L and s_R from nine variables it took from the caller's scope. Extracting only the signal speed keeps the min/max wave structure at the call site, where it reads as the physics, instead of hiding it: s_L = min(vel_L(dir_idx(1)) - f_elastic_signal_speed(c_L, G_L, tau_e_L(...), rho_L), ...) compute_hypo_elastic_energy took three arguments but still read i, tau_e_L, tau_e_R, G_L and G_R implicitly, so its signature implied a contract it did not keep. f_elastic_energy returns the increment and the accumulation is explicit; the shear test becomes a named shear_cond rather than an expression passed as a macro argument. Both live in m_riemann_state, which all three solvers already use. Hypoelasticity 59 passed, 0 failed. Static resources unchanged except the same single 16 B of scratch in m_riemann_solver_hlld already present before these two, which is the MFlowCode#1759 whole-image variance and does not grow with them. Part of MFlowCode#1769.
sbryngelson
merged commit Aug 28, 2026
cd40b57
into
fix/speed-of-sound-from-state
61 of 63 checks passed
Owner
Author
|
Folded into MFlowCode#1762 — these commits are now on that branch, so this closed itself when it was pushed. The macro work was always stacked on Landed there: 6 of 9 |
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.
Turns the fypp macros that expand into a caller's scope into ordinary procedures with explicit
interfaces. First three of the nine in MFlowCode#1769; six of the twenty-six call sites.
A zero-argument macro reads and assigns the local variables of whatever routine expands it, so a
call site says nothing about what it touches and neither the compiler nor a reader can check the
coupling. MFlowCode#1769 has the worked example:
roe_avgaccumulatesvel_avg_rmsover every velocitycomponent and overwrites it with the first component six lines later, which is a visible double
assignment in a subroutine and invisible inside a 45-line macro.
In this PR
hll_flux_componentf_hll_flux(S_L, S_R, F_L_i, F_R_i, U_L_i, U_R_i)compute_capillary_stress_tensors_compute_capillary_stress_tensor(sigma, w1, w2, w3, normW, Omega)compute_axis_inv_res_compute_axis_inv_re(grad_x_vf, grad_y_vf, grad_z_vf, alpha_visc, j, k, l, Re_visc)include/inline_capillary.fppheld nothing but the second macro and is deleted with it.Two details worth review rather than assumption:
Omegaisintent(inout), notintent(out). The macro assigned only the entries a givendimensionality defines, leaving the rest as the caller had them;
intent(out)would make themundefined in 1D and 2D.
gamma_dot_cwas live only insidecompute_axis_inv_re, so it becomes a local of the new routineand leaves the caller's declarations and all four
private()clauses.Cost
compute_axis_inv_retakes threescalar_fieldarray dummies on a[seq]device routine, which isthe shape MFlowCode#1714 showed collapsing register promotion across a whole loop body for 20%. It does not
reproduce. The distinction appears to be real: MFlowCode#1714 constructed a derived type and took its
address, while these arrays already exist and are passed by reference.
viscous_weno5_sgb_acoustic, MI210, AFAR amdflang, OpenMP offload, three runs each on one node:-0.50%, with every after run below every before run - consistent with the smaller
private()clauses rather than with luck.
The other two macros have no benchmark that reaches them: no benchmark case enables surface tension,
and
f_hll_fluxlives in the hypoelastic HLLD solver while the only hypoelastic benchmark uses HLL.For those the evidence is the offload image: identical scratch, VGPR and AGPR across all 471 kernels,
which is the mechanism a non-inlined call would have to show up in.
One kernel does move:
m_riemann_solver_hlldgains 16 B of scratch on 21440, in a module none ofthese changes can reach. That is the whole-image codegen variance of MFlowCode#1759. Every kernel actually
touched here is unchanged.
Tests
Hypoelasticity 59, Viscous 63, the four capillary and surface_tension cases - 126 passed, 0 failed.
No goldens regenerated.
Remaining
Six macros, twenty call sites:
compute_low_Mach_correction(8, and it silently overwritesvel_L(dir_idx(1))andvel_R(dir_idx(1))),compute_elastic_wave_speeds_lr(2),compute_hypo_elastic_energy(1), andarithmetic_avg/roe_avg/compute_average_state(5), whichare widest and should go last - the chemistry path there produces six further outputs and wants its
own routine.
Whether
roe_avg's dead loop should lose the loop or the overwrite is a physics question, not arefactoring one, and belongs in its own change.
Stacked on MFlowCode#1762 because
m_riemann_solver_hypo_hlld.fppis not on master yet.