From 0d7b6cddbaf5e4b65fc69b4513512e69260b4ed0 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 2 Nov 2026 20:27:07 -0500 Subject: [PATCH] Fix bc_type dummy shape and write serial BC files every save; no-MPI scalar reductions return their input; fail format on ffmt structure warnings; Cray debug bounds checking; Open MPI oversubscribe; stage-guard lint; validator fixes --- cmake/GPU.cmake | 2 ++ docs/documentation/contributing.md | 8 ++++++-- src/common/m_boundary_io.fpp | 8 ++------ src/common/m_mpi_common.fpp | 6 ++++++ src/pre_process/m_data_output.fpp | 5 +++-- src/simulation/m_data_output.fpp | 27 +++++++++++++++------------ toolchain/bootstrap/format.sh | 15 +++++++++++++-- toolchain/mfc/case_validator.py | 13 +++++++++---- toolchain/mfc/lint_source.py | 25 +++++++++++++++++++++++++ toolchain/mfc/test_analytic_expr.py | 4 ++-- toolchain/mfc/test_lint_source.py | 13 +++++++++++++ toolchain/templates/default.mako | 6 ++++++ 12 files changed, 102 insertions(+), 30 deletions(-) diff --git a/cmake/GPU.cmake b/cmake/GPU.cmake index 44cda01cdc..79b12369af 100644 --- a/cmake/GPU.cmake +++ b/cmake/GPU.cmake @@ -90,9 +90,11 @@ elseif (CMAKE_Fortran_COMPILER_ID STREQUAL "Cray") add_link_options("SHELL:-hkeepfiles") if (CMAKE_BUILD_TYPE STREQUAL "Debug") + # -h bounds: Cray array-bounds/pointer checking, matching the other compilers' debug branches above. add_compile_options( "SHELL:-h acc_model=auto_async_none" "SHELL: -h acc_model=no_fast_addr" + "SHELL: -h bounds" "SHELL: -K trap=fp" "SHELL: -g" "SHELL: -O0" ) add_link_options("SHELL: -K trap=fp" "SHELL: -g" "SHELL: -O0") diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index d86d61fefd..2f7eb136e9 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -113,8 +113,12 @@ reconfigure. **Fypp and per-target stubs.** Fypp resolves `#:include` at parse time, so every `.fpp` file sees exactly the include path for the target being compiled. `src/common/` is -compiled once per executable — this is intentional. The per-target include path is what -lets common modules include per-target generated files without duplication. +compiled once per executable, and that include path is how common modules pick up the +per-target generated files. Do not gate code on the executable: the per-executable +preprocessor guards (`MFC_PRE_PROCESS`, `MFC_SIMULATION`, `MFC_POST_PROCESS`) were +removed from the sources and the source lint rejects them. Stage-varying behaviour is +passed in as an argument or an initialization policy, and stage-only code lives in that +stage's directory. For which files are manual vs. generated and what each contains, see the "How to Add a New Simulation Parameter" section below. diff --git a/src/common/m_boundary_io.fpp b/src/common/m_boundary_io.fpp index 0699a50530..0942f95c98 100644 --- a/src/common/m_boundary_io.fpp +++ b/src/common/m_boundary_io.fpp @@ -60,17 +60,15 @@ contains end subroutine s_create_mpi_types - !> Write boundary condition type and buffer data to serial (unformatted) restart files. - subroutine s_write_serial_boundary_condition_files(q_prim_vf, bc_type, step_dirpath, old_grid_in, q_T_sf) + !> Write bc type and bc_buffers to serial restart files (pre_process packs them first; simulation writes the startup values). + subroutine s_write_serial_boundary_condition_files(bc_type, step_dirpath, old_grid_in) - type(scalar_field), dimension(sys_size), intent(in) :: q_prim_vf type(integer_field), dimension(1:num_dims,1:2), intent(in) :: bc_type logical, intent(in) :: old_grid_in character(LEN=*), intent(in) :: step_dirpath integer :: dir, loc character(len=path_len) :: file_path character(len=10) :: status - type(scalar_field), optional, intent(in) :: q_T_sf if (old_grid_in) then status = 'old' @@ -78,8 +76,6 @@ contains status = 'new' end if - call s_pack_boundary_condition_buffers(q_prim_vf, q_T_sf) - file_path = trim(step_dirpath) // '/bc_type.dat' open (1, FILE=trim(file_path), form='unformatted', STATUS=status) do dir = 1, num_dims diff --git a/src/common/m_mpi_common.fpp b/src/common/m_mpi_common.fpp index c54568d276..f835162a16 100644 --- a/src/common/m_mpi_common.fpp +++ b/src/common/m_mpi_common.fpp @@ -373,6 +373,8 @@ contains integer :: ierr !< Generic flag used to identify and report MPI errors call MPI_ALLREDUCE(var_loc, var_glb, 1, mpi_p, MPI_SUM, MPI_COMM_WORLD, ierr) +#else + var_glb = var_loc #endif end subroutine s_mpi_allreduce_sum @@ -440,6 +442,8 @@ contains integer :: ierr !< Generic flag used to identify and report MPI errors call MPI_ALLREDUCE(var_loc, var_glb, 1, mpi_p, MPI_MIN, MPI_COMM_WORLD, ierr) +#else + var_glb = var_loc #endif end subroutine s_mpi_allreduce_min @@ -470,6 +474,8 @@ contains integer :: ierr !< Generic flag used to identify and report MPI errors call MPI_ALLREDUCE(var_loc, var_glb, 1, mpi_p, MPI_MAX, MPI_COMM_WORLD, ierr) +#else + var_glb = var_loc #endif end subroutine s_mpi_allreduce_max diff --git a/src/pre_process/m_data_output.fpp b/src/pre_process/m_data_output.fpp index 98e8c187ef..72063349b4 100644 --- a/src/pre_process/m_data_output.fpp +++ b/src/pre_process/m_data_output.fpp @@ -88,10 +88,11 @@ contains if (bc_io) then if (igr) then - call s_write_serial_boundary_condition_files(q_cons_vf, bc_type, t_step_dir, old_grid) + call s_pack_boundary_condition_buffers(q_cons_vf) else - call s_write_serial_boundary_condition_files(q_prim_vf, bc_type, t_step_dir, old_grid, q_T_sf) + call s_pack_boundary_condition_buffers(q_prim_vf, q_T_sf) end if + call s_write_serial_boundary_condition_files(bc_type, t_step_dir, old_grid) end if file_loc = trim(t_step_dir) // '/x_cb.dat' diff --git a/src/simulation/m_data_output.fpp b/src/simulation/m_data_output.fpp index 1a72bc6ede..ad01aa39cc 100644 --- a/src/simulation/m_data_output.fpp +++ b/src/simulation/m_data_output.fpp @@ -19,6 +19,7 @@ module m_data_output use m_delay_file_access use m_ibm use m_boundary_common + use m_boundary_io, only: s_write_serial_boundary_condition_files use m_constants, only: model_eqns_5eq, precision_single implicit none @@ -57,12 +58,12 @@ contains !> Write data files. Dispatch subroutine that replaces procedure pointer. impure subroutine s_write_data_files(q_cons_vf, q_T_sf, q_prim_vf, t_step, bc_type, beta) - type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf - type(scalar_field), intent(inout) :: q_T_sf - type(scalar_field), dimension(sys_size), intent(inout) :: q_prim_vf - integer, intent(in) :: t_step - type(scalar_field), intent(inout), optional :: beta - type(integer_field), dimension(1:num_dims,-1:1), intent(in) :: bc_type + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf + type(scalar_field), intent(inout) :: q_T_sf + type(scalar_field), dimension(sys_size), intent(inout) :: q_prim_vf + integer, intent(in) :: t_step + type(scalar_field), intent(inout), optional :: beta + type(integer_field), dimension(1:num_dims,1:2), intent(in) :: bc_type if (.not. parallel_io) then call s_write_serial_data_files(q_cons_vf, q_T_sf, q_prim_vf, t_step, bc_type, beta) @@ -485,7 +486,7 @@ contains type(scalar_field), dimension(sys_size), intent(inout) :: q_prim_vf integer, intent(in) :: t_step type(scalar_field), intent(inout), optional :: beta - type(integer_field), dimension(1:num_dims,-1:1), intent(in) :: bc_type + type(integer_field), dimension(1:num_dims,1:2), intent(in) :: bc_type character(LEN=path_len + 2*name_len) :: t_step_dir !< Relative path to the current time-step directory character(LEN=path_len + 3*name_len) :: file_path !< Relative path to the grid and conservative variables data files logical :: file_exist !< Logical used to check existence of current time-step directory @@ -499,6 +500,8 @@ contains call my_inquire(file_path, file_exist) if (file_exist) call s_delete_directory(trim(t_step_dir)) call s_create_directory(trim(t_step_dir)) + ! Serial readers (post_process, restart) need bc_type.dat and bc_buffers.dat in every step directory, not just step 0. + if (bc_io) call s_write_serial_boundary_condition_files(bc_type, t_step_dir, .false.) file_path = trim(t_step_dir) // '/x_cb.dat' @@ -832,11 +835,11 @@ contains !> Write grid and conservative variable data files in parallel via MPI I/O impure subroutine s_write_parallel_data_files(q_cons_vf, t_step, bc_type, beta, q_T_sf) - type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf - integer, intent(in) :: t_step - type(scalar_field), intent(inout), optional :: beta - type(integer_field), dimension(1:num_dims,-1:1), intent(in) :: bc_type - type(scalar_field), intent(inout), optional :: q_T_sf + type(scalar_field), dimension(sys_size), intent(inout) :: q_cons_vf + integer, intent(in) :: t_step + type(scalar_field), intent(inout), optional :: beta + type(integer_field), dimension(1:num_dims,1:2), intent(in) :: bc_type + type(scalar_field), intent(inout), optional :: q_T_sf #ifdef MFC_MPI integer :: ifile, ierr, data_size diff --git a/toolchain/bootstrap/format.sh b/toolchain/bootstrap/format.sh index 962fe5d7a9..2feeec78be 100644 --- a/toolchain/bootstrap/format.sh +++ b/toolchain/bootstrap/format.sh @@ -51,11 +51,22 @@ else PYTHON_DIRS="toolchain/ examples/ benchmarks/" fi -# Format Fortran files with ffmt (single-pass, idempotent) -if ! ffmt -j ${JOBS:-1} $FORTRAN_DIRS 2>/dev/null; then +# Format Fortran files with ffmt (single-pass, idempotent). +# ffmt prints unmatched-structure warnings to stderr yet exits 0: show stderr and treat them as fatal. +FFMT_ERR="$(mktemp)" +trap 'rm -f "$FFMT_ERR"' EXIT +ffmt -j ${JOBS:-1} $FORTRAN_DIRS 2>"$FFMT_ERR" +FFMT_RC=$? +[ -s "$FFMT_ERR" ] && cat "$FFMT_ERR" >&2 +if [ $FFMT_RC -ne 0 ]; then error "Formatting Fortran files failed: ffmt." exit 1 fi +# match the construct name, not the bare word, which a diagnostic or path could carry +if grep -q "unmatched Fortran" "$FFMT_ERR"; then + error "ffmt reported unmatched Fortran block structure: the source will not compile." + exit 1 +fi # Apply safe auto-fixes (import sorting, etc.) before formatting. # --fix-only exits 0 even when unfixable violations remain — those are diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index 2167c3f749..149aaabcdb 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -1246,6 +1246,7 @@ def check_mhd(self): self.prohibit(mhd and num_fluids != 1, "MHD is only available for single-component flows (num_fluids = 1)") self.prohibit(mhd and model_eqns != 2, "MHD is only available for the 5-equation model (model_eqns = 2)") + self.prohibit(mhd and self.get("bubbles_euler", "F") == "T", "MHD is not available with bubbles_euler (the HLLD mixture coefficients are single-fluid stiffened gas)") self.prohibit(relativity and not mhd, "relativity requires mhd to be enabled") pi_inf = self.get("fluid_pp(1)%pi_inf") self.prohibit( @@ -1295,7 +1296,8 @@ def check_riemann_solver(self): def check_time_stepping(self): """Checks time stepping parameters (simulation/post-process)""" - cfl_dt = self.get("cfl_dt", "F") == "T" + # mirrors the Fortran derivation (m_start_up.fpp): cfl_adap_dt or cfl_const_dt sets cfl_dt + cfl_dt = any(self.get(k, "F") == "T" for k in ("cfl_dt", "cfl_adap_dt", "cfl_const_dt")) cfl_adap_dt = self.get("cfl_adap_dt", "F") == "T" time_stepper = self.get("time_stepper") @@ -2164,8 +2166,7 @@ def check_chemistry(self): # inconsistent and the simulation NaNs. See MFlowCode/MFC#1470. self.prohibit(chemistry and num_fluids is not None and num_fluids != 1, "chemistry is only supported for single-component flows (num_fluids = 1)") - # The diffusion flux arrays are allocated only under chemistry, but the Riemann-state and - # RHS loops test chem_params%diffusion alone, so without chemistry they write unallocated arrays. + # The source-flux buffer is sized from component 1 only when chemistry and diffusion are both on, but consumers key on diffusion alone. self.prohibit(diffusion and not chemistry, "chem_params%diffusion requires chemistry = T") # Chemistry with Euler bubbles is not currently supported: the IBM image-point @@ -2291,7 +2292,11 @@ def check_reactive_burn(self): # sentinel in the solver and silently corrupts the burn, so require each to be set. rk = self.get("rburn%k") self.prohibit(not self._is_numeric(rk) or rk <= 0, "reactive_burn requires rburn%k > 0 (rate coefficient [1/s])") - self.prohibit(self.get("rburn%pign") is None, "reactive_burn requires rburn%pign to be set (ignition pressure threshold [Pa])") + rpign = self.get("rburn%pign") + self.prohibit( + not self._is_numeric(rpign) or rpign <= 0, + "reactive_burn requires rburn%pign > 0 (ignition pressure threshold [Pa]); unset, or any " "non-positive value, ignites the reactant everywhere from t = 0", + ) rpref = self.get("rburn%pref") self.prohibit(not self._is_numeric(rpref) or rpref <= 0, "reactive_burn requires rburn%pref > 0 (it normalizes the pressure drive and is used as a divisor)") rn = self.get("rburn%n") diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index 98962222c3..cf8c04aef5 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -209,6 +209,30 @@ def check_raw_directives(repo_root: Path) -> list[str]: return errors +def check_stage_guards(repo_root: Path) -> list[str]: + """Flag per-executable preprocessor guards (MFC_PRE_PROCESS, MFC_SIMULATION, MFC_POST_PROCESS) in source. + + Stage-varying behaviour is passed as an argument; stage-only code lives in that stage's directory. + """ + errors: list[str] = [] + src_dir = repo_root / SRC_DIR + guard_re = re.compile(r"\bMFC_(PRE_PROCESS|SIMULATION|POST_PROCESS)\b") + + for src in _fortran_fpp_files(src_dir): + lines = src.read_text(encoding="utf-8").splitlines() + rel = src.relative_to(repo_root) + for i, line in enumerate(lines): + stripped = line.strip() + if _is_comment_or_blank(stripped): + continue + if guard_re.search(stripped): + errors.append( + f" {rel}:{i + 1} per-executable guard {guard_re.search(stripped).group(0)}. " "Fix: move stage-only code into that stage's directory or pass the behaviour in as an argument" + ) + + return errors + + def check_double_precision(repo_root: Path) -> list[str]: """Flag double-precision-specific intrinsics and type declarations. @@ -786,6 +810,7 @@ def main(): all_errors: list[str] = [] all_errors.extend(check_raw_directives(repo_root)) + all_errors.extend(check_stage_guards(repo_root)) all_errors.extend(check_double_precision(repo_root)) all_errors.extend(check_junk_code(repo_root)) all_errors.extend(check_false_integers(repo_root)) diff --git a/toolchain/mfc/test_analytic_expr.py b/toolchain/mfc/test_analytic_expr.py index 268fc93e51..b5ecb06c6d 100644 --- a/toolchain/mfc/test_analytic_expr.py +++ b/toolchain/mfc/test_analytic_expr.py @@ -95,7 +95,7 @@ def test_exponent_literal_gets_wp_suffix(): # Corpus-equivalence gate: AST translator vs legacy regex substitution # NOTE: intrinsics (sin, cos, ...) and pi must NOT be var_map keys, or the legacy reference's pass-through behavior diverges and the gate silently degrades. -# Harvested 2026-06-09 from examples/*/case.py with: +# Harvested from examples/*/case.py with: # grep -rhoE '"patch_icpp\([0-9]+\)%(alpha_rho|vel|pres|alpha|tau_e|Y|cf_val|Bx|By|Bz)(\([0-9]+\))?"\s*:\s*"[^"]+"' examples/*/case.py | sort -u # grep -rhoE '"patch_ib\([0-9]+\)%(vel|angular_vel)\([0-9]+\)"\s*:\s*"[^"]+"' examples/*/case.py | sort -u # Excluded: pure-number values (e.g. "0"), T/F booleans, file paths, and enum @@ -110,7 +110,7 @@ def rhs_replace(match): CORPUS = [ - # harvested 2026-06-09 from examples/*/case.py (see plan Task 2 Step 1) + # harvested as above "0.5 + 0.2 * sin(2.0 * pi * x / lx)", "0.5 - 0.5*sin(2*pi*x)", "0.5 + 0.5*sin(2*pi*x)", diff --git a/toolchain/mfc/test_lint_source.py b/toolchain/mfc/test_lint_source.py index 53b177c1fd..9f6575fc10 100644 --- a/toolchain/mfc/test_lint_source.py +++ b/toolchain/mfc/test_lint_source.py @@ -7,6 +7,7 @@ check_double_precision, check_integer_wp, check_manual_registry_bcasts, + check_stage_guards, ) BCAST_TAIL = ", 1, mpi_p, 0, MPI_COMM_WORLD, ierr)" @@ -61,6 +62,18 @@ def _write_proxy(tmp_path, target_dir: str, body: str): _write_src(tmp_path, f"{target_dir}/m_mpi_proxy.fpp", body) +def test_stage_guards_flag_the_per_executable_defines(tmp_path): + _write_src(tmp_path, "common/m_x.fpp", "#ifdef MFC_SIMULATION\n call s_only_here()\n#endif\n") + errors = check_stage_guards(tmp_path) + assert len(errors) == 1 + assert "MFC_SIMULATION" in errors[0] + + +def test_stage_guards_ignore_comments_and_other_gates(tmp_path): + _write_src(tmp_path, "simulation/m_x.fpp", " ! MFC_SIMULATION is only a CMake option\n#ifdef MFC_GPU\n#endif\n") + assert check_stage_guards(tmp_path) == [] + + def test_double_precision_flags_signed_d_exponent(tmp_path): _write_src(tmp_path, "simulation/m_x.fpp", " if (x > 5.0d-11) then\n") errors = check_double_precision(tmp_path) diff --git a/toolchain/templates/default.mako b/toolchain/templates/default.mako index 2d05d3ab3c..4a688a768e 100644 --- a/toolchain/templates/default.mako +++ b/toolchain/templates/default.mako @@ -56,6 +56,12 @@ if engine == 'batch': srun --ntasks ${nodes*tasks_per_node} \ "${target.get_install_binpath(case)}") elif [ "$binary" == "mpirun" ]; then + # Open MPI caps ranks at the physical core count; these lift the cap (Open MPI 4 and 5). Other launchers ignore them. + avail=$(nproc 2> /dev/null || echo 0) + if [ "$avail" -gt 0 ] && [ ${nodes*tasks_per_node} -gt "$avail" ]; then + echo "mfc: WARNING: ${nodes*tasks_per_node} ranks on $avail available processors - oversubscribing (ranks share cores; timings are not representative)." + fi + export OMPI_MCA_rmaps_base_oversubscribe=1 PRTE_MCA_rmaps_default_mapping_policy=:oversubscribe (set -x; ${profiler} \ $binary -np ${nodes*tasks_per_node} \ "${target.get_install_binpath(case)}")