Skip to content

Refactor variable conversion into stage-independent common code - #1679

Open
sbryngelson wants to merge 19 commits into
MFlowCode:masterfrom
sbryngelson:refactor/common-stage-independence
Open

Refactor variable conversion into stage-independent common code#1679
sbryngelson wants to merge 19 commits into
MFlowCode:masterfrom
sbryngelson:refactor/common-stage-independence

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Jul 26, 2026

Copy link
Copy Markdown
Member

Description

This contains the first incremental steps toward making src/common genuinely stage-independent instead of selecting different implementations with MFC_SIMULATION, MFC_PRE_PROCESS, and MFC_POST_PROCESS.

The first slice focuses on m_variables_conversion:

  • removes all stage-selection macros from the module;
  • uses one host/device scalar species-to-mixture conversion kernel in every executable;
  • makes primitive-to-conservative, primitive-to-flux, sound-speed, and fast-magnetosonic routines consistently available;
  • moves material-property arrays, viscosity-index metadata, and direction mappings to the common global-parameter module so they have one declaration and GPU ownership point;
  • replaces stage-dependent behavior with explicit initialization policies:
    • simulation enables the density floor and QBMM number preservation;
    • post-process requests mixture diagnostic storage and provides its Lagrangian beta index;
    • pre-process uses the common defaults;
  • allocates and updates conversion data consistently, including Re_idx, Re_size, material arrays, and the new policy scalars.

The second slice makes shared grid-buffer population stage-independent:

  • m_boundary_common now receives cell-boundary, cell-center, and cell-width arrays explicitly instead of selecting stage-global representations;
  • simulation supplies its uniform buff_size cell-boundary extent, while post-process supplies its asymmetric Silo/output offsets;
  • the one-dimensional physical-boundary algorithm is shared across x, y, and z instead of dispatching to direction-specific global-array routines;
  • MPI grid-buffer exchange has an explicit one-dimensional array interface;
  • simulation retains ownership of glb_bounds and performs the GPU update immediately after the host-side grid population.

The third slice removes the superseded grid implementations and separates minimum spacing from width-array storage:

  • deletes the old direction-specific physical-boundary routines and module-global MPI grid exchange;
  • introduces common dx_min, dy_min, and dz_min scalars;
  • changes pre-process grid construction and smoothing calculations to use the explicitly named minimum spacings;
  • leaves simulation/post-process dx, dy, and dz width arrays unchanged;
  • removes the simulation/pre-process selection from STL model grid-spacing logic.

The fourth slice makes MPI I/O field binding and downsampled datatype construction stage-independent:

  • passes QBMM pressure and moment fields explicitly instead of selecting pb/mv or pb_ts(1)/mv_ts(1) by stage;
  • passes immersed-boundary marker storage and its MPI descriptor together, with a runtime consistency check;
  • passes downsampled local extents explicitly instead of deriving different layouts from the executable stage;
  • makes downsampled conservative-field pointer binding optional, so pre-process/simulation can construct file datatypes while post-process can additionally bind its input fields;
  • removes six more stage guards from m_mpi_common, reducing the remaining stage-condition count in src/common from 30 to 24.

The fifth slice makes Dirichlet ghost filling an explicit runtime policy:

  • replaces the simulation/post-process/pre-process implementation guard in s_dirichlet;
  • initializes simulation to consume stored boundary buffers while pre-process and post-process retain boundary-cell extrapolation;
  • declares and updates the policy scalar through the existing GPU portability macros so the shared GPU_ROUTINE takes the same branch on host and device;
  • reduces the remaining stage-condition count in src/common from 24 to 23.

The sixth slice removes low-risk stage selection from common validation and finite differences:

  • makes the total-cell check an explicit caller policy, with pre-process and post-process supplying nGlobal while simulation skips the check;
  • removes empty stage guards from the common checker;
  • makes callers own finite-difference coefficient allocation instead of reallocating only for post-process inside the shared coefficient routine;
  • reduces the remaining stage-condition count in src/common from 23 to 19.

The seventh slice moves common global declarations, equation indexing, and GPU declarations to explicit ownership:

  • generates num_dims, num_vels, weno_polyn, and muscl_polyn declarations for all executables, while preserving simulation case optimization;
  • emits GPU declarations beside the generated simulation declarations that own the corresponding variables;
  • keeps case-optimized parameters as compile-time constants without creating device storage for them;
  • replaces the six-equation eqn_idx%alf stage guard with an explicit initialization policy, preserving simulation, pre-process, and post-process behavior;
  • removes the stage branch from common dimension and velocity initialization;
  • reduces the remaining stage-condition count in src/common from 19 to 14.

The eighth slice makes common MPI transport stage-independent:

  • makes unified-memory preference depend only on NVIDIA unified-memory capability;
  • compiles the shared stability reduction for every executable while retaining MPI/non-MPI capability selection;
  • replaces the simulation/post-process chemistry halo branch with an explicit temperature-exchange policy;
  • sizes the chemistry halo buffer using the same policy, including post-processing without diffusion;
  • shares one staged/RDMA send-receive implementation for ordinary and Lagrangian-beta halos;
  • initializes RDMA explicitly from simulation and disables it for pre-process/post-process;
  • reduces the remaining stage-condition count in src/common from 14 to 9.

The ninth slice makes MPI domain decomposition stage-independent and removes its repeated directional policy code:

  • passes Silo ghost-offset and local-domain-adjustment policies explicitly;
  • uses thin pre-process and post-process adapters to pack stage-owned x/y/z structures into common three-direction arrays;
  • computes interior Silo offsets for all active dimensions in one shared loop;
  • computes local pre-process domain bounds and minimum spacings for all active dimensions in one shared loop;
  • leaves topology selection, remainder distribution, parallel-I/O start indices, neighbor ranks, and boundary-rank construction unchanged;
  • reduces the remaining stage-condition count in src/common from 9 to 3.

The tenth slice makes common STL model setup available in every executable:

  • removes the post-process exclusion around s_instantiate_STL_models;
  • keeps one shared implementation of model loading, transformation, bounding boxes, two-dimensional boundary extraction, GPU packing, and device updates;
  • extends generated declaration ownership for num_stl_models and stl_models to post-process without adding either variable to its namelist;
  • teaches typed generated declarations to honor declaration targets independently of input targets;
  • reduces the remaining stage-condition count in src/common from 3 to 1.

The eleventh slice makes the full common phase-change module available in every executable:

  • removes the post-process exclusion around the module imports, public interface, and solver implementations;
  • keeps the existing pT/pTg relaxation algorithms, tolerances, failure handling, and accelerator loops unchanged;
  • extends declaration ownership for palpha_eps and ptgalpha_eps to post-process without adding them to its namelist;
  • preserves the current asserting behavior of the otherwise-empty relaxation dispatcher;
  • removes the final executable-stage conditional from src/common.

Design choices

Stage identity is no longer used as a proxy for accelerator execution. Code inside accelerator regions selects the device-callable kernel with MFC_GPU; CPU execution uses the host wrapper, which can also populate optional post-processing diagnostics.

The scalar conversion kernel is marked with GPU_ROUTINE and is shared by CPU and accelerator paths. Device-resident module data retains explicit GPU_DECLARE, enter-data, update, and delete lifecycle operations. Material arrays and viscosity metadata are declared only once, in their common owning module.

Some variables must now be declared in all three executables because common code references them, but they should not become accepted inputs in every namelist. The parameter generator therefore separates Fortran declaration targets from namelist targets. Existing user-facing scopes for avg_state, alt_soundspeed, mixture_err, sigR, viscous, and riemann_solver remain unchanged.

This change deliberately does not remove capability guards such as MFC_GPU or backend-specific GPU macros. It removes stage-based implementation selection.

Pre-process currently uses dx, dy, and dz as scalar minimum spacings, whereas simulation and post-process use those names for width arrays. Passing the grid representation explicitly allows the common boundary implementation to compile for every executable without hiding that semantic mismatch behind another stage macro.

That mismatch is resolved in the third slice: scalar minimum spacings now have distinct common names, while per-cell width arrays retain the established dx, dy, and dz names.

MPI I/O now follows the same ownership rule: common code constructs views and binds only the objects supplied by its caller. Paired optional data are checked together, preventing a marker without a descriptor or one QBMM node field without the other. Accelerator ownership is unchanged because these routines set host-side MPI pointer/view metadata and introduce no device-data movement.

Dirichlet behavior is configured when the boundary module is initialized, rather than inferred from the executable name. The policy is device resident because s_dirichlet is called inside accelerator boundary loops.

Finite-difference routines now compute into caller-provided storage. This keeps allocation and lifetime with the executable that owns the array and avoids hiding post-process reallocation inside common numerical code.

Generated declarations are now the ownership boundary for generated simulation inputs. The generator emits exactly one GPU_DECLARE for each device-resident scalar it declares. Manually declared common state retains direct portability macros in the common module. Capability guards such as MFC_GPU, OpenACC, and OpenMP remain intact; only executable-stage selection is being removed.

MPI transport now has one implementation. Executables configure whether all chemistry temperatures need halo exchange and whether RDMA transport is enabled; the common layer owns buffer sizing and selects the corresponding transport path. OpenACC/OpenMP data movement remains expressed through the portability macros.

MPI decomposition owns the Cartesian algorithm, while executables provide only the data and policies that are actually stage-specific. Pre-process supplies mutable domain bounds and requests local-domain adjustment for generated grids; post-process supplies mutable output offsets and requests Silo overlap; simulation disables both policies. The common implementation applies both policies dimensionally, eliminating the three copies of each formula.

STL instantiation remains a single common routine. Post-process now has the declarations needed to compile that routine, but no new accepted inputs and no new call site. Generated declaration targets are therefore the compile-time ownership mechanism, while namelist targets remain the user-facing input contract.

Phase-change follows the same ownership rule: the complete numerical implementation is compiled once in common for all executables, while only the stages that currently need it invoke it. The two solver tolerances are declaration-visible in post-process solely so the common module compiles; their accepted input scope remains pre-process/simulation.

Behavior changes

This is predominantly a refactor, but three changes are not behavior-preserving and are called out deliberately.

Unifying the mixture-conversion kernel means pre_process and post_process now apply the mpp_lim clamp and volume-fraction renormalization that previously existed only in the simulation path. For mpp_lim = T this can change derived quantities wherever the stored volume fractions are slightly out of bounds or do not sum to one. The full golden-file suite, including post-process comparisons, shows no movement in any covered case.

Unifying the chemistry halo policy fixes a latent buffer overrun on master: buffer allocation used chemistry .and. chem_params%diffusion while the exchange itself used present(q_T_sf) .and. chemistry for pre-process and post-process, so a chemistry run without diffusion packed nVar + 1 variables into buffers sized for sys_size. Both sites now share one predicate.

Consequently pre_process no longer exchanges the ghost temperature layer when chemistry is enabled without diffusion. Nothing consumes it: elliptic smoothing reads only q_prim_vf, and s_compute_T_from_primitives recomputes the temperature over the interior immediately afterwards.

Type of change

  • Refactor

Testing

  • Built pre_process, simulation, and post_process without MPI.

  • Built all three targets with MPI.

  • Built a case-optimized simulation using examples/1D_euler_convergence/case.py.

  • Ran focused MPI regression cases with post-processing:

    • 8EAC3DA7 (1D viscous)
    • 9DAC4DDC (two-fluid alternative sound speed)
    • CE9DBA3F (QBMM)
    • B9553426 (Lagrangian bubbles)
  • Ran an end-to-end non-MPI 1D_euler_convergence example.

  • Ran focused grid and boundary regressions with post-processing:

    • B49877F3 (3D grid stretching)
    • 8C7AA13B (2D, two MPI ranks)
    • DB670E50 (axisymmetric boundary)
    • 3008BA80 (periodic immersed-boundary case)
  • Ran focused STL/model regressions:

    • EA8FA07E (2D IBM STL)
    • E777BE53 (3D pre-process ICPP STL)
  • Ran focused MPI I/O regressions:

    • 356A5C50 (non-polytropic QBMM with parallel I/O)
    • 3008BA80 (immersed-boundary marker I/O)
  • Ran a dedicated MPI downsampling smoke case through pre-process, simulation, and post-process; an 11x11x11 input was read as the expected 3x3x3 post-process domain.

  • Ran 986BC1A2 (2D Richtmyer--Meshkov with Dirichlet boundary-buffer input) through pre-process, simulation, and post-process with MPI.

  • Built a case-optimized simulation for examples/2D_richtmyer_meshkov/case.py.

  • Ran FD891191 (two-fluid six-equation model) through pre-process, simulation, and post-process with MPI.

  • Ran 1550B67E (2D chemistry with diffusion disabled) through pre-process, simulation, and post-process with MPI.

  • Re-ran B9553426 (2D Lagrangian bubbles) through all three stages with MPI to exercise ordinary and beta halo transport.

  • Re-ran decomposition regressions through pre-process, simulation, and post-process with MPI:

    • B49877F3 (3D grid stretching)
    • 8C7AA13B (2D, two MPI ranks, Silo output)
    • DB670E50 (axisymmetric)
  • Ran a two-rank old-grid smoke from examples/1D_vacuum_restart: generated the original parallel grid, then re-read it with old_grid = T in a temporary case directory. The temporary restart copy supplied domain bounds required by current validation and disabled old_ic so the check exercised grid reuse without requiring a 7000-step restart file.

  • Re-ran STL regressions:

    • EA8FA07E (2D IBM STL)
    • E777BE53 (3D pre-process ICPP STL)
  • Ran phase-change regressions through pre-process, simulation, and post-process with MPI:

    • E6DBCBD9 (1D, model 5, two fluids, six-equation model)
    • 02BD9B5E (2D, model 6, three fluids, six-equation model)
    • 344FAA70 (3D, model 5, two fluids, six-equation model)
  • Ran the parameter generator tests: 93 passed.

  • Ran the focused Fortran-generator tests after the declaration changes: 48 passed.

  • Ran the repository pre-commit gate: formatting, spelling, toolchain lint, source lint, documentation references, parameter documentation, and all example validations passed.

  • Expanded the affected sources for OpenACC and inspected the resulting declarations and data lifecycle. The device conservative-to-primitive path calls the GPU_ROUTINE kernel directly, and the old stage-specific accelerator routine is absent.

  • Expanded the grid changes for both OpenACC and OpenMP. The host grid population precedes acc update device(glb_bounds) / omp target update to(glb_bounds), and common grid routines do not take over device-data ownership.

  • Expanded m_mpi_common for all three executables with both OpenACC and OpenMP definitions and inspected the new explicit QBMM, immersed-boundary, and downsampled interfaces. A physical accelerator build is unavailable on this macOS/GNU toolchain.

  • Expanded the Dirichlet change for all three executables with both OpenACC and OpenMP definitions. The policy receives acc declare / omp declare target and acc update device / omp target update, and both buffer-copy and extrapolation paths remain in the device routine.

  • Expanded the common global-parameter module for all three executables with both OpenACC and OpenMP definitions. Pre-process/post-process receive only common manual device declarations; simulation receives exactly one generated declaration for each owned GPU scalar.

  • Expanded a case-optimized simulation and verified that num_dims, num_vels, weno_polyn, and muscl_polyn remain Fortran parameters with no OpenACC/OpenMP device allocation.

  • Expanded m_mpi_common for all three executables with both OpenACC and OpenMP definitions after transport unification. Every stage receives the same staged and RDMA branches; runtime policy selects one, and the portability macros emit the expected host/device updates or device-address regions.

  • Re-expanded m_mpi_common for every executable and both accelerator backends after decomposition unification; each expansion contains the same single policy helper and no stage-selection residue.

  • Built a case-optimized MPI simulation for examples/2D_richtmyer_meshkov/case.py after the decomposition interface change.

  • Expanded m_model for all three executables with OpenACC and OpenMP definitions. Every expansion contains exactly one STL-instantiation routine, its existing device updates, and no stage-selection residue.

  • Expanded m_phase_change for all three executables with OpenACC and OpenMP definitions. Every expansion contains the same full phase-change solver and accelerator loop annotations, with no stage-selection residue.

  • Verified that rg -n 'MFC_(PRE_PROCESS|SIMULATION|POST_PROCESS)' src/common returns no matches.

  • Merged master (Remove non-operational hyperelasticity capability #1675, hyperelasticity removal) and resolved eight conflicts; m_hyperelastic.fpp is deleted, elasticity becomes hypoelasticity, and b_size/tensor_size/elasticity GPU declarations are dropped.

  • Ran the full golden-file suite with post-process comparisons enabled (./mfc.sh test -a) on GNU 13.3 with MPI: 577 tests, no golden-file differences.

  • Built all three targets with NVHPC 25.11 and OpenACC, and ran the suite on NVIDIA A100 hardware: 296 cases from the full run plus a 30-case set targeting every device-facing change (IBM STL, immersed-boundary markers, Lagrangian bubbles, non-polytropic QBMM, cylindrical and axisymmetric grids, phase change, mpp_lim, viscous, and the boundary-condition sweep). No failures.

  • Ran both chemistry cases on GPU (1550B67E, C4EB58A8) after building the chemistry variant.

  • Built a case-optimized simulation with NVHPC and OpenACC, and confirmed the generated declarations emit case-optimization parameters as Fortran parameter constants with no device storage, while the non-case-optimized branch emits exactly the twenty-two GPU_DECLARE names the deleted hand-written block contained.

  • syscheck passes with MPI and OpenACC device offload on the same machine.

Cray and Intel are covered only by CI; neither was exercised locally.

Checklist

  • I added or updated tests for new behavior
  • I updated documentation if user-facing behavior changed

No user-facing parameter behavior changed; the declaration/namelist separation preserves the existing accepted inputs for each executable. The numerical changes are limited to those listed under Behavior changes.

See the developer guide for full coding standards.

GPU changes (expand if you modified src/simulation/)
  • GPU results match CPU results
  • Tested on NVIDIA GPU or AMD GPU

Built with NVHPC 25.11 and OpenACC and run on NVIDIA A100 hardware. The GPU suite covers every device-facing change in this PR and reports no failures; the same cases pass on the CPU build against the same golden files. AMD and OpenMP-offload builds were not exercised.

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)
  • Or add label claude-full-review — Claude full review via label

Resolves conflicts with MFlowCode#1675 (hyperelasticity removal):
- accept deletion of src/simulation/m_hyperelastic.fpp
- adopt hypoelasticity in place of the removed elasticity flag
- drop GPU_DECLARE for elasticity, b_size, tensor_size
- drop hyperelasticity from SIM_GPU_DECL_VARS, whose parameter
  master removed (silent auto-merge break; failed the generator test)
Collapse the optional-argument dispatch in s_convert_species_to_mixture_variables: absent optional dummies forward as absent, so the four branches were unnecessary. Default dirichlet_from_buffers so it is defined before module initialization. Fail generation when SIM_GPU_DECL_VARS names a variable simulation does not declare, instead of silently dropping its device residency. Drop a stray build log and refresh the stage-independence notes.
COMMON_CASE_OPT_EXTRA_NAMES silently emitted nothing for a name that CASE_OPT_EXTRA_LINES does not define, leaving pre_process and post_process without a declaration that common code references. Raise instead, matching the SIM_GPU_DECL_VARS guard.
@sbryngelson
sbryngelson marked this pull request as ready for review July 26, 2026 20:08
Copilot AI review requested due to automatic review settings July 26, 2026 20:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is an incremental refactor to make src/common/ genuinely stage-independent by removing MFC_SIMULATION/MFC_PRE_PROCESS/MFC_POST_PROCESS-based implementation selection and replacing it with explicit runtime policies and explicit data ownership/arguments. It also updates the parameter generator to separate “declared in Fortran” from “accepted in a stage namelist” so common code can compile for all executables without expanding user-facing inputs.

Changes:

  • Update the Fortran parameter generator to support declaration targets independent of namelist targets, and to centralize simulation scalar GPU residency via SIM_GPU_DECL_VARS.
  • Make common modules stage-independent via explicit policies/arguments (variables conversion, boundary/grid buffering, decomposition, MPI I/O binding, Dirichlet behavior, finite differences, STL setup, phase change).
  • Rename scalar minimum spacings to dx_min/dy_min/dz_min and propagate usage across pre_process and common IC/model logic.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
toolchain/mfc/params/generators/fortran_gen.py Adds declaration-target support and simulation GPU_DECLARE ownership enforcement.
toolchain/mfc/params/definitions.py Introduces DECLARATION_TARGETS to decouple Fortran visibility from namelist acceptance.
toolchain/mfc/params_tests/test_fortran_gen.py Adds tests ensuring declarations don’t expand namelists and that GPU_DECLARE ownership rules hold.
src/simulation/m_start_up.fpp Passes explicit common policies (checker, MPI common, variables conversion, boundary common, decomposition) and explicit grid-buffer population.
src/simulation/m_sim_helpers.fpp Switches to the shared species→mixture conversion kernel.
src/simulation/m_ibm.fpp Switches to the shared species→mixture conversion kernel.
src/simulation/m_global_parameters.fpp Removes sim-local ownership of arrays moved to common; updates initialization policy calls.
src/simulation/m_data_output.fpp Updates MPI I/O initialization calls to new explicit interfaces (includes downsample path).
src/simulation/m_cbc.fpp Switches to the shared species→mixture conversion kernel.
src/simulation/m_bubbles_EL.fpp Switches to the shared species→mixture conversion kernel.
src/pre_process/m_start_up.fpp Uses explicit common checker/decomposition policies and renames min spacings to *_min.
src/pre_process/m_icpp_patches.fpp Updates smoothing logic to use dx_min/dy_min/dz_min.
src/pre_process/m_grid.f90 Renames grid-spacing scalar usage to dx_min/dy_min/dz_min and updates calculations accordingly.
src/pre_process/m_global_parameters.fpp Removes pre-process scalar dx/dy/dz declarations and redundant defaults now owned in common.
src/pre_process/m_data_output.fpp Updates downsample MPI I/O initialization and explicit QBMM binding in MPI I/O.
src/post_process/m_start_up.fpp Uses explicit common checker/decomposition policies and explicit grid-buffer population.
src/post_process/m_global_parameters.fpp Adjusts defaults now owned in common and updates eqn index init policy.
src/post_process/m_data_input.f90 Updates MPI I/O initialization for the new explicit IB descriptor interface and downsample signature.
src/common/m_variables_conversion.fpp Removes stage selection, introduces explicit conversion policies, and shares one host/device conversion kernel.
src/common/m_phase_change.fpp Enables the full phase-change module to compile for all executables.
src/common/m_mpi_common.fpp Makes MPI I/O binding and transport/decomposition stage-independent with explicit policies and interfaces.
src/common/m_model.fpp Makes STL instantiation available in all executables and switches to *_min spacings.
src/common/m_global_parameters_common.fpp Centralizes shared declarations, adds dx_min/dy_min/dz_min, and replaces stage guards with explicit init policies.
src/common/m_finite_differences.fpp Removes post-process-only allocation side effects from shared FD coefficient routines.
src/common/m_checker_common.fpp Replaces stage-based total-cell checking with explicit caller policy and explicit n_global argument.
src/common/m_boundary_primitives.fpp Makes Dirichlet ghost-filling behavior a runtime policy via a device-resident flag.
src/common/m_boundary_common.fpp Refactors grid-buffer population to an explicit-array interface shared across dimensions and stages.
src/common/include/macros.fpp Makes unified-memory preference depend on capability rather than stage.
src/common/include/3dHardcodedIC.fpp Updates hardcoded IC logic to use dx_min.
src/common/include/2dHardcodedIC.fpp Updates hardcoded IC logic to use dx_min/dy_min.
.claude/rules/common-pitfalls.md Updates documented “common pitfalls” guidance to match the new stage-independent pattern and generator ownership.
Comments suppressed due to low confidence (1)

src/simulation/m_data_output.fpp:718

  • The unconditional s_initialize_mpi_data(q_cons_vf, ...) call overwrites any downsample-specific binding of MPI_IO_DATA%var established earlier, which can cause downsampled writes (especially in the bubbles_euler path) to use full-resolution buffers. Guard this call so it does not run when down_sample is active.
            call s_initialize_mpi_data(q_cons_vf, qbmm_pb=pb_ts(1), qbmm_mv=mv_ts(1))

Comment thread src/simulation/m_data_output.fpp
riemann_solver is not a case-optimization parameter, so its default belongs outside the case-optimization guard; case-optimized simulation builds otherwise lost the dflt_int sentinel. post_process declares viscous for the common conversion kernel but neither accepts it as input nor broadcasts it, and assigns defaults on rank 0 only, so give it a value on every rank. Initialize the MPI transport policies, keep global_bounds intent(inout) so inactive dimensions retain their defaults, and mark the grid-width buffer contiguous where an element is passed to MPI.
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 65.01650% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.77%. Comparing base (83bf84c) to head (86ea1d9).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/common/m_mpi_common.fpp 58.44% 16 Missing and 16 partials ⚠️
src/common/m_variables_conversion.fpp 42.85% 17 Missing and 15 partials ⚠️
src/common/m_boundary_common.fpp 84.09% 0 Missing and 7 partials ⚠️
src/pre_process/m_start_up.fpp 58.82% 7 Missing ⚠️
src/pre_process/m_grid.f90 77.77% 6 Missing ⚠️
src/pre_process/m_icpp_patches.fpp 16.66% 5 Missing ⚠️
src/common/m_model.fpp 0.00% 3 Missing ⚠️
src/common/m_checker_common.fpp 50.00% 1 Missing and 1 partial ⚠️
src/post_process/m_data_input.f90 33.33% 2 Missing ⚠️
src/simulation/m_data_output.fpp 71.42% 2 Missing ⚠️
... and 7 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1679      +/-   ##
==========================================
- Coverage   61.04%   60.77%   -0.28%     
==========================================
  Files          83       83              
  Lines       20978    20872     -106     
  Branches     3099     3101       +2     
==========================================
- Hits        12807    12685     -122     
+ Misses       6126     6121       -5     
- Partials     2045     2066      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…NaNs)

CCE 19.0.0 silently drops intent(inout) stores to a non-contiguous assumed-shape array dummy when the same call also passes a contiguous one. s_mpi_sendrecv_grid_variable_buffer marked only cell_widths contiguous, so the ghost values of x_cb and x_cc were never written at rank seams. s_initialize_weno_module then builds reconstruction coefficients from x_cb across that ghost range, with x_cb differences in the denominators, so every multi-rank case grew a NaN within ~50 steps.

Multi-rank only because the routine runs only for bc_edge >= 0. Cray GPU only because CMakeLists applies -Oipa0 to the whole simulation target on CPU but to just four files on GPU, and the caller m_boundary_common is not one of them; the failure tracks the caller's IPA level. Declaring every array dummy on the chain contiguous avoids the mixed-attribute call. All actual arguments are whole module allocatables, so the attribute is accurate.

Verified on Frontier with CCE 19.0.0 and OpenMP offload: all 8 multi-rank tests pass (0FCCE9F1, 8C7AA13B, CE232828, AFBACA70, CB0DC420, 75D7CC39, 0090B316, 0DDE8A87), previously all NaN.
…owdown)

Moving dir_idx/dir_flg/dir_idx_tau from simulation's m_global_parameters into m_global_parameters_common cost ~18 kB/work-item of register spill in every dimensionally-split kernel that reads them under AMD OpenMP offload. m_riemann_solver_hll.fpp, whose source this PR never touches, went 828 B -> 18832 B of scratch, VGPR 72 -> 128 (ceiling), 100 ms -> 1430 ms per direction - about 100% of the regression. s_convert_primitive_to_flux_variables went 60 B -> 17888 B. Kernels that never read the mapping (weno, 28 B both ways) or that were already VGPR-saturated on master (s_convert_conservative_to_primitive_variables, 18272 B both ways) were unaffected: the slowdown tracks dir_idx/dir_flg reference count exactly - 45 refs 14x, 10 refs 11x, 0 refs 1.00x.

Declare the mapping in simulation as on master and pass it explicitly into the single common consumer, s_convert_primitive_to_flux_variables (called only from m_cbc). src/common keeps zero stage guards and now holds no reference to simulation direction state.

Measured on hpcfund gfx90a (MI250X), amdflang AFAR 23.2.0, OpenMP offload, 1 rank, benchmarks/5eq_rk3_weno3_hll --gbpp 4, rocprofv3 kernel trace: grind ns/gp/eq/rhs master 3.128 | PR 7.460 | PR+fix 3.068; total device time 1995 | 5969 | 1951 ms; Riemann scratch 828 | 18832 | 828 B. Local suite was 186/627 with 0 failures at push time; full run and CCE coverage via CI.
… NVHPC build)

copyinReadOnly emits OpenACC copyin(readonly: ...); nvfortran rejects it with NVFORTRAN-S-0034-Syntax error at or near : and the gpu-acc lanes failed to build m_variables_conversion. The OpenMP path emits map(to: ...) for both spellings, so the amdflang build I validated on could not see it. copyin is the spelling already used in-tree (m_time_steppers, m_body_forces); no other call site uses copyinReadOnly on a parallel loop.

Verified by generating the directives with fypp: !$acc& copyin(dir_idx_in, dir_flg_in) and !$omp& map(to:dir_idx_in, dir_flg_in). The OMP text is unchanged, so the measured 3x recovery from 89c25e3 stands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants