Fixed per-thread array bounds on all GPU compilers, with a generated state-vector layout - #1941
Open
sbryngelson wants to merge 6 commits into
Open
sbryngelson wants to merge 6 commits into
sbryngelson wants to merge 6 commits into
Conversation
…itch
Per-thread arrays in GPU kernels get compile-time extents because
runtime-sized private arrays spill to scratch (4-5x on amdflang). The 83
copy-pasted `#:if [not MFC_CASE_OPTIMIZATION and] USING_AMD` blocks become
single declarations using ${BOUND('name')}$, which returns the fixed maximum
from one table (num_fluids 3, nb 3, sys_size 70 or 10 + species, ...) or the
runtime extent. A converter checked every literal against that table.
MFC_FIXED_BOUNDS (CMake, default ON) is decided per target: only the
offloaded simulation uses fixed bounds, so pre/post_process callers always
match the routines they call. This commit keeps it amdflang-only, and the
generated code for amdflang simulation and for every non-fixed build matches
master. s_check_amd becomes s_check_fixed_bounds and absorbs the CBC limits.
…table toolchain/mfc/params/eqn_layout.py lists each model's fields in order with their size and condition as small Python expressions. The params codegen translates them into generated_eqn_idx.fpp, which s_initialize_eqn_idx now includes in place of 140 hand-written lines (the hypoelastic shear tables stay hand-written). evaluate_layout() computes the same layout for a case, which case optimization will use for a compile-time sys_size. Checked against master's routine over all 7,962,624 combinations of model, the 13 layout flags, 1D/multi-D, and the fluid/velocity/dimension/bubble/ species counts: every eqn_idx field and sys_size match, and evaluate_layout agrees on sys_size for a 300,000-case sample.
Case optimization now evaluates the layout table for the case and writes CASE_OPT_SIZES (sys_size and the hypoelastic stress count) into case.fpp. BOUND() returns those exact values, or the case-optimized parameters, under case optimization on any compiler; before, sys_size arrays fell back to the runtime value or, on amdflang, to the fixed maximum of 70. The sys_size variable itself stays a runtime variable filled by the generated layout, so nothing that declares or updates it on the device changes. Simulation checks at startup that the runtime layout matches the baked-in values. CASE_OPT_SIZES is part of case.fpp, so it is already in the build fingerprint: a case with a different layout gets its own build.
MFC_FIXED_BOUNDS now applies to NVHPC and CCE (OpenACC and OpenMP), not just amdflang. CPU builds keep runtime extents: with GNU the fixed bounds cost up to 39% on the 5-equation cases. In MFlowCode#1938's CI benchmarks the fixed bounds sped up CCE by 12-36% and NVHPC OpenMP by 9-62%, with AMD unchanged. GPU simulation builds without case optimization now take at most 3 fluids, nb <= 3, and sys_size <= 70 (or 10 + species); larger cases rebuild with --case-optimization, and s_check_fixed_bounds says so. Docs describe BOUND() and why runtime-sized per-thread arrays are slow on GPUs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fixed-size GPU dummy arguments conflict with shorter host-side simulation arrays in common one- and two-fluid paths.
Review effort: Balanced
Findings: 2
Open (4)
What changed in this PR
Centralizes GPU per-thread array bounds and generates equation-state layouts for consistent runtime and case-optimized sizing.
Changes:
- Introduces
BOUND()with GPU maxima and case-specific extents. - Generates
eqn_idxandsys_sizefrom a shared Python layout. - Updates validation, tests, CMake code generation, and documentation.
| File | Description |
|---|---|
toolchain/mfc/params/generators/fortran_gen.py |
Generates equation-index includes. |
toolchain/mfc/params/eqn_layout.py |
Defines and evaluates state-vector layouts. |
toolchain/mfc/params_tests/test_fortran_gen.py |
Updates generated-file tests. |
toolchain/mfc/params_tests/test_eqn_layout.py |
Tests layout evaluation and translation. |
toolchain/mfc/lint_source.py |
Renames the fixed-bound checker exemption. |
toolchain/mfc/case.py |
Computes case-optimized extents. |
src/simulation/m_weno.fpp |
Applies fixed WENO scratch bounds. |
src/simulation/m_viscous.fpp |
Applies fluid and dimension bounds. |
src/simulation/m_time_steppers.fpp |
Bounds timestep scratch arrays. |
src/simulation/m_surface_tension.fpp |
Bounds capillary tensors. |
src/simulation/m_start_up.fpp |
Validates baked and maximum sizes. |
src/simulation/m_sim_helpers.fpp |
Bounds helper arrays. |
src/simulation/m_riemann_state.fpp |
Bounds Riemann-state scratch tensors. |
src/simulation/m_riemann_solver_lf.fpp |
Bounds LF solver arrays. |
src/simulation/m_riemann_solver_hypo_hlld.fpp |
Bounds hypoelastic HLLD arrays. |
src/simulation/m_riemann_solver_hlld.fpp |
Bounds HLLD fluid arrays. |
src/simulation/m_riemann_solver_hllc.fpp |
Bounds HLLC state arrays. |
src/simulation/m_riemann_solver_hll.fpp |
Bounds HLL fluid arrays. |
src/simulation/m_qbmm.fpp |
Bounds QBMM scratch arrays. |
src/simulation/m_pressure_relaxation.fpp |
Bounds relaxation arrays. |
src/simulation/m_ibm.fpp |
Bounds immersed-boundary scratch arrays. |
src/simulation/m_data_output.fpp |
Bounds runtime diagnostic arrays. |
src/simulation/m_compute_cbc.fpp |
Bounds CBC helper arguments. |
src/simulation/m_cbc.fpp |
Bounds CBC working arrays. |
src/simulation/m_bubbles_EL.fpp |
Bounds Lagrangian-bubble arrays. |
src/simulation/m_bubbles_EE.fpp |
Bounds Eulerian-bubble arrays. |
src/simulation/m_acoustic_src.fpp |
Bounds acoustic-source fluid arrays. |
src/common/m_variables_conversion.fpp |
Bounds conversion-kernel arrays. |
src/common/m_phase_change.fpp |
Bounds and initializes phase arrays. |
src/common/m_global_parameters_common.fpp |
Consumes generated equation layouts. |
src/common/m_eos.fpp |
Bounds EOS helper arguments. |
src/common/m_checker_common.fpp |
Enforces fixed-bound limits. |
src/common/include/shared_parallel_macros.fpp |
Defines centralized bound selection. |
docs/documentation/gpuParallelization.md |
Documents BOUND() usage. |
docs/documentation/contributing.md |
Updates generated-file documentation. |
CMakeLists.txt |
Adds the fixed-bounds option. |
cmake/ParamsCodegen.cmake |
Registers generated layout files. |
cmake/Fypp.cmake |
Enables fixed bounds per target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+645
to
+648
| real(wp), intent(in) :: pres, rho, gamma, pi_inf | ||
| real(wp), dimension(${BOUND('num_fluids')}$), intent(in) :: adv | ||
| real(wp), intent(out) :: c | ||
| real(wp), dimension(${BOUND('num_fluids')}$), intent(in), optional :: alpha_rho |
Member
Author
There was a problem hiding this comment.
Fixed in 41a73ae. The probe-output locals (s_write_probe_files) and s_report_icfl_violation's locals are now ${BOUND('num_fluids')}$. The ICFL path was worse than nonconforming: it reaches the conversion kernel via s_compute_cell_state, whose whole-array alpha_K normalization wrote past a one- or two-fluid host array under mpp_lim (see the other thread).
Comment on lines
+190
to
+191
| real(wp), dimension(${BOUND('num_fluids')}$), intent(inout) :: alpha_rho_K, alpha_K | ||
| real(wp), optional, dimension(${BOUND('num_fluids')}$), intent(in) :: G |
Member
Author
There was a problem hiding this comment.
Fixed in 41a73ae. The wrapper's alpha_K/alpha_rho_K locals and both wrappers' optional G now use ${BOUND('num_fluids')}$; every caller that passes G passes fluid_pp(:)%G (sized num_fluids_max), so that stays conforming. The kernel also now normalizes only alpha_K(1:num_fluids): the whole-array form wrote past a short host actual with mpp_lim, and divided the uninitialized padding even with a conforming one.
| registers a single ninja-tracked `add_custom_command` (DEPENDS all `params/*.py`) that | ||
| invokes `cmake_gen.py` and writes the 18 generated includes under | ||
| invokes `cmake_gen.py` and writes the 21 generated includes under | ||
| `build/include/<target>/`. There is no configure-time generation: all 18 files are build |
Member
Author
There was a problem hiding this comment.
Fixed in 41a73ae: 21 throughout (matches the list in cmake/ParamsCodegen.cmake).
| @:PROHIBIT(hypoelasticity .and. eqn_idx%stress%end - eqn_idx%stress%beg + 1 /= ${CASE_OPT_SIZES['n_stress']}$, & | ||
| & "stress count differs from the case-optimized build; rebuild it") | ||
| #:elif MFC_FIXED_BOUNDS | ||
| @:PROHIBIT(sys_size > ${SYS_SIZE_MAX}$, "sys_size <= ${SYS_SIZE_MAX}$ in GPU builds") |
Member
Author
There was a problem hiding this comment.
Fixed in 41a73ae: the message now ends with "; rebuild with --case-optimization", like the num_fluids/nb checks.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1941 +/- ##
==========================================
- Coverage 62.80% 62.77% -0.04%
==========================================
Files 86 86
Lines 22387 22295 -92
Branches 3306 3291 -15
==========================================
- Hits 14061 13995 -66
+ Misses 6071 6060 -11
+ Partials 2255 2240 -15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 task
# Conflicts: # docs/documentation/gpuParallelization.md
Lines of Code
|
In GPU simulation builds BOUND('num_fluids') is 3, but three host paths still
passed num_fluids-sized arrays to routines with BOUND dummies:
s_convert_species_to_mixture_variables (its alpha_K/alpha_rho_K locals and the
forwarded G), s_report_icfl_violation, and s_write_probe_files. With one or two
fluids and mpp_lim, the kernel's whole-array normalization of alpha_K wrote past
the end of the host local. Declare those with BOUND (every G caller passes
fluid_pp(:)%G, sized num_fluids_max), and normalize only alpha_K(1:num_fluids).
Also: fix the generated-file count in contributing.md (21, not 18), and tell
users to rebuild with --case-optimization when sys_size exceeds the GPU maximum.
This branch has not been deployed
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.
Summary
Per-thread arrays in GPU kernels now get compile-time extents in every GPU simulation build, from one place, and case optimization supplies exact extents including
sys_size.Runtime-sized private arrays (
dimension(num_fluids),dimension(sys_size)) cannot live in registers and spill to scratch. TheUSING_AMDguards fixed this for amdflang only; profiling on AFAR 24.3 (rocprofv3) showed WENO 11x and HLLC 4.9x slower without them. #1938 turned the same fixed bounds on for every compiler, and CI showed CCE 12-36% and NVHPC OpenMP 9-62% faster (AMD unchanged), but GNU CPU up to 39% slower. So this PR applies them to GPU builds only.Changes (four commits)
BOUND()replaces theUSING_AMDguards. The 83 copy-pasted#:if [not MFC_CASE_OPTIMIZATION and] USING_AMDblocks become single declarations such asdimension(${BOUND('num_fluids')}$).BOUNDreturns the fixed maximum from one table (num_fluids3,nb3,sys_size70 or 10 + species, WENO and QBMM extents, and so on) or the runtime extent. A converter checked every existing literal against that table.MFC_FIXED_BOUNDS(CMake, default ON) is decided per target: only the offloadedsimulationuses fixed bounds, sopre_process/post_processcallers always match the routines they call.s_check_amdbecomess_check_fixed_bounds, and thesys_sizelimit is checked right after the layout is computed (the old input-time check ran beforesys_sizeexisted).toolchain/mfc/params/eqn_layout.pylists each model's fields with their size and condition as small Python expressions. The params codegen turns them intogenerated_eqn_idx.fpp, replacing about 140 hand-written lines ofs_initialize_eqn_idx. Checked against master's routine over all 7,962,624 combinations of model, the 13 layout flags, 1D/multi-D, and the counts: everyeqn_idxfield andsys_sizematches.CASE_OPT_SIZES(sys_sizeand the hypoelastic stress count) intocase.fpp, soBOUNDgives exact compile-time extents under case optimization on every compiler. Thesys_sizevariable stays a runtime variable, so nothing that declares or updates it on the device changes. Simulation checks at startup that the runtime layout matches the baked-in values. SinceCASE_OPT_SIZESis incase.fpp, it is part of the build fingerprint.BOUND().Behavior change
GPU simulation builds without
--case-optimizationnow accept at most 3 fluids,nb <= 3, andsys_size <= 70(or 10 + species) on every GPU compiler, as amdflang already did; larger cases rebuild with--case-optimization, and the error message says so. CPU builds andpre_process/post_processare unaffected.Testing
simulation, and for gfortransimulation/pre_process/post_process, the preprocessed Fortran matches master apart from the checker rename and messages.--gpu mp, release: full test suite, 718/718 passed.5eq_rk3_weno3_hllcon GPU: runs withCASE_OPT_SIZES = {'sys_size': 8}baked in (3D, two fluids), HLLC and CBC arrays sized exactly 8, startup check passes; grind 1.93 against 2.50 without case optimization (--mem 2).--mem 2): geometric-mean ratio 1.000, every case within ±2%. AMD already had fixed bounds, so no change is expected; the gains are on NVHPC and CCE.This PR was prepared with Claude Code (AI-assisted).
Acknowledgement