Share the global-parameters core across executables; generate MPI broadcasts; build hygiene

[sbryngelson]

Stacked on #1551 — this branch includes those commits; the diff below them is this PR's own content (14 commits). Review after (or alongside) #1551; it will rebase cleanly once #1551 merges.

Summary

Third installment of the parameter/declaration single-source-of-truth work (#1550, #1551). Two thrusts:

1. Executable dedup. The three per-executable m_global_parameters.fpp shared 50–90% of their content by copy-paste. A new src/common/m_global_parameters_common.fpp now owns the generated declaration includes, the shared state (eqn_idx, sys_size, b_size, tensor_size, shear bookkeeping), the equation-index setup (s_initialize_eqn_idx), parallel-IO init, the finalize core, and the common default assignments. The per-target modules keep only what genuinely differs (QBMM moment encodings, GPU update blocks, post's output state — each leave-behind verified divergent-by-need). The three m_mpi_proxy.fpp broadcast lists are now generated from the parameter registry (generated_bcast.fpp per target): a registered namelist parameter is broadcast by construction, with only computed variables (m_glb, cfl_dt, bc_io, …) remaining hand-listed. Net src/ change: −477 LOC.

2. Build hygiene. CMakeLists.txt (1,135 lines) splits into a ~460-line root plus cmake/{GPU,Fypp,ParamsCodegen,MFCTargets}.cmake — pure motion, verified by byte-identical configure graphs. The generated includes (now 15: namelist, decls, constants, case-opt, bcast × 3 targets) regenerate via a single ninja-tracked custom command; the configure-time bootstrap and its mtime stamp are deleted (editing params/*.py + rebuild now always yields a correct binary, no reconfigure). Build variant directories get readable names: build/install/gpu-acc-chem-f93aa400b9 instead of de835ec46c (hash inputs unchanged — variant identity preserved).

Bug fix (deliberate behavior change #1)

post_process never broadcast chem_wrt_Y despite consuming it on all ranks in s_save_data — silent wrong/missing species output for multi-rank chemistry post-processing. The registry-driven generator fixes this by construction. (Broadcast tuple-set deltas vs. before, verified per target: simulation identical; pre_process adds only the dead-but-registered n_start_old; post_process adds only chem_wrt_Y.)

Deliberate behavior change #2

post_process's beta_idx (Lagrangian bubble void fraction) is now appended after the MHD/elasticity/surface-tension blocks, matching simulation's writer layout — the previous post-only ordering misread restart data for bubbles_lagrange combined with those features. Covered by the full golden suite.

Verification

• Full golden suite (sharded) green on the dedup stack and again on the final rebased tip (post-Add Herschel-Bulkley non-Newtonian viscosity model #1545), including the chemistry tests that exercise per-case rebuilds.
• Emitted-.f90 declaration-set and GPU-directive-set equivalence per target for every hoist step; broadcast tuple-set equivalence per target; configure-graph byte-equivalence for the CMake split; cold-start + incremental-regeneration proofs for the generation rework.

Notes for reviewers

• src/common continues to compile once per executable with MFC_<TARGET> defines — deliberate and now documented in contributing.md ("How the Build Fits Together"); it is what enables per-target generated includes and the shared module pattern.
• A follow-up PR (broadcast correctness) fixes three pre-existing multi-rank broadcast bugs found while auditing the generated lists; it is stacked on this branch.
• Old hash-named build directories orphan until ./mfc.sh clean.

[github-actions]

PR #1552 Review

HEAD SHA: 525e0bd
Scope: CMakeLists refactor, params code-generation, shared m_global_parameters_common, WENO/MUSCL constant naming.

---

Summary

A large structural refactor with clear benefits: CMakeLists.txt is split into four focused cmake/ includes, three per-target m_global_parameters.fpp files lose duplicated boilerplate, and the hand-written MPI broadcast loops across all three executables are replaced with generated code. The architecture and intent are sound. One MPI correctness regression is present in the generated broadcast code that must be fixed before merge.

---

Critical — MPI Correctness Regression

Dropped fluid_pp member broadcasts for non-Newtonian fluids

File: toolchain/mfc/params/generators/fortran_gen.py, function _emit_fluid_pp

The generated fluid_pp broadcast loop in all three targets now broadcasts only six members:

fp_real_members = ["gamma", "pi_inf", "G", "cv", "qv", "qvp"]

The previous hand-written loops in all three m_mpi_proxy.fpp files explicitly broadcast eight additional members per fluid:

Member	Type	Purpose
K	real	Herschel-Bulkley consistency index
nn	real	power-law exponent
tau0	real	yield stress
hb_m	real	Herschel-Bulkley smoothing exponent
mu_min	real	lower viscosity bound
mu_max	real	upper viscosity bound
mu_bulk	real	bulk viscosity
non_newtonian	logical	non-Newtonian feature flag

These members are read directly from the namelist on rank 0 only; they must be broadcast to reach all other MPI ranks. Since fluid_pp is in TYPED_DECLS, _classify_scalar_vars skips it entirely — the only path for these members is through _emit_fluid_pp. None of the three targets emit a broadcast for them.

Effect: In any multi-rank run with non_newtonian = .true., non-zero ranks have non_newtonian = .false. and all Herschel-Bulkley parameters at their defaults. The simulation produces wrong answers with no error or warning.

Fix: Add the missing members to fp_real_members (and emit non_newtonian separately as MPI_LOGICAL):

fp_real_members = ["gamma", "pi_inf", "G", "cv", "qv", "qvp",
                   "K", "nn", "tau0", "hb_m", "mu_min", "mu_max", "mu_bulk"]

Then emit the logical member after the real loop:

lines.append(f"    call MPI_BCAST(fluid_pp(i)%non_newtonian, 1, MPI_LOGICAL, 0, MPI_COMM_WORLD, ierr)")

The companion unit test in test_fortran_gen.py should be extended to assert these members appear for all three targets.

---

Minor Observations

Comment formatting in m_constants.fpp

Line 53 reads:

! Reconstruction Types Interface Compression

This appears to be two section headings merged without a separator, dropping the blank line that formerly separated the "Reconstruction Types" constants from the "Interface Compression" constants. Not a correctness issue, but a blank comment line between them restores the intent.

---

What Looks Good

• CMakeLists.txt split into cmake/GPU.cmake, cmake/Fypp.cmake, cmake/MFCTargets.cmake, cmake/ParamsCodegen.cmake is clean. Build-time codegen via ninja custom commands replaces configure-time execute_process, which is the correct pattern.
• m_global_parameters_common.fpp correctly guards all case-optimization declarations with #:if not MFC_CASE_OPTIMIZATION, and the stub generated for pre/post confirms no double-declaration risk.
• Named constants (recon_type_weno, recon_type_muscl, etc.) replacing magic literals WENO_TYPE = 1/MUSCL_TYPE = 2 are an unambiguous improvement, properly sourced from generated_constants.fpp.
• s_assign_common_defaults and the partial-initialization pattern (common then per-target) correctly replicate the original default assignment order.
• build.py slug now includes a human-readable prefix; no correctness impact.
• The manual MPI residue kept in the three m_mpi_proxy.fpp files (grid dimensions, BC structs, cfl_dt, bc_io, shear/body forces, lag/bub/chem guards) matches what is genuinely not auto-generatable.

---

Recommendation

Request changes. The dropped fluid_pp non-Newtonian broadcasts are a silent correctness regression for multi-rank non-Newtonian runs. Fix _emit_fluid_pp, extend the unit test to cover the missing members for all three targets, then this is ready to merge.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[sbryngelson]

Confirmed and fixed in e4bb56a: the hardcoded fluid_pp emitter predated the #1545 merge and dropped the Herschel-Bulkley members (K, nn, tau0, hb_m, mu_min, mu_max, mu_bulk, non_newtonian) from the generated broadcast lists. The emitted sets are now verified member-identical to master's manual lists for all three targets, and the test suite pins the HB members so this class of escape fails loudly. The stacked follow-up PR (#1553) additionally converts this emitter to a registry walk, making new fluid_pp members broadcast by construction. Good catch.

[sbryngelson]

Both review items now addressed on this branch: the fluid_pp broadcast regression (e4bb56a, member-identical to master's manual lists, test-pinned) and the merged m_constants section heading (dac3d1f). Thanks for the thorough review.

[sbryngelson]

Code review

Found 1 issue (beyond the two from the earlier automated review, both already addressed):

1. NIB reads the wrong Fortran constant: num_ib_patches_max (2,050,000) instead of num_ib_patches_max_namelist (54,000), while the patch_ib namelist array is dimensioned by the latter — so case validation accepted patch_ib indices that overflow the Fortran array. The adjacent comment even claims it enforces the namelist limit. Fixed in f42e532 along with three doc passages made stale by the dedup.

MFC/toolchain/mfc/params/definitions.py

Lines 33 to 35 in dac3d1f

	NUM_PATCHES_MAX = _fc("num_patches_max", 10) # patch_icpp (Fortran array bound)
	NIB = _fc("num_ib_patches_max", 54000) # patch_ib (Fortran array bound)
	NAF = _fc("num_ib_airfoils_max", 5) # ib_airfoil (Fortran array bound)

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}