Raise the amdflang species bound so mechanisms above ten species build - #1852
Raise the amdflang species bound so mechanisms above ten species build#1852sbryngelson wants to merge 7 commits into
Conversation
The USING_AMD guard substitutes a literal extent for dimension(num_species) when case optimization is off, and that literal was ten across 21 declarations in nine files, with m_checker_common stating the matching limit. A mechanism above ten species cannot be built that way, and the check meant to explain it never runs: m_thermochem.f90 is generated per mechanism with concrete bounds, so the mismatch is a compile error first and the reader gets a wall of argument-mismatch diagnostics instead of the sentence. Measured on a 2D reacting mixing layer, generic chemistry GPU build on MI210: the HLLC kernel goes from 260 to 326 registers and 396 to 1372 bytes of scratch per thread, and the case runs 1.37e-1 to 1.40e-1 seconds per step, about two percent. The wave count does not move, both rounding to one wave per SIMD. rhoYks in m_variables_conversion used a different spelling of the same bound and is included.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Raises the hardcoded amdflang (non-case-optimized) species array extent so chemistry mechanisms with >10 species can compile, and aligns the documented num_species limit check with the new bound.
Changes:
- Increased multiple
USING_AMD/not MFC_CASE_OPTIMIZATIONarray extents from 10 to 20 across chemistry/CBC/IBM and Riemann modules. - Updated the amdflang configuration checker to prohibit
num_species > 20instead of> 10. - Normalized one previously hardcoded
(1:10)extent to(1:20)in variables conversion.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulation/m_riemann_state.fpp | Bumps amdflang non-case-optimized species array extents to 20 in Riemann state helper. |
| src/simulation/m_riemann_solver_lf.fpp | Updates Lax–Friedrichs solver local species arrays to the new padded bound. |
| src/simulation/m_riemann_solver_hllc.fpp | Updates HLLC solver local thermochemistry/species arrays to the new padded bound. |
| src/simulation/m_riemann_solver_hll.fpp | Updates HLL solver local species arrays to the new padded bound. |
| src/simulation/m_ibm.fpp | Updates IBM path species array extent for amdflang non-case-optimized builds. |
| src/simulation/m_compute_cbc.fpp | Updates CBC helper interfaces taking dYs_ds to use extent 20 under the amdflang guard. |
| src/simulation/m_cbc.fpp | Updates CBC routine local species/thermo arrays to the new padded bound. |
| src/common/m_variables_conversion.fpp | Updates hardcoded species temporary arrays to extent 20 under the amdflang guard. |
| src/common/m_chemistry.fpp | Updates chemistry kernels’ local species arrays to the new padded bound. |
| src/common/m_checker_common.fpp | Aligns the enforced species limit message/constraint with the new bound (20). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| !! from here: CCE faults the GPU on that call one routine deeper. | ||
| #:if not MFC_CASE_OPTIMIZATION and USING_AMD | ||
| real(wp), dimension(10), intent(in) :: Ys_L, Ys_R, R_species, h_iL, h_iR, Cp_iL, Cp_iR | ||
| real(wp), dimension(20), intent(in) :: Ys_L, Ys_R, R_species, h_iL, h_iR, Cp_iL, Cp_iR |
|
|
||
| #:if not MFC_CASE_OPTIMIZATION and USING_AMD | ||
| real(wp), dimension(10) :: Yi_avg, Phi_avg, h_avg_2 | ||
| real(wp), dimension(20) :: Yi_avg, Phi_avg, h_avg_2 |
| @:PROHIBIT(num_fluids > 3, "num_fluids <= 3 for AMDFLang when Case optimization is off") | ||
| @:PROHIBIT((bubbles_euler .or. bubbles_lagrange) .and. nb > 3, "nb <= 3 for AMDFLang when Case optimization is off") | ||
| @:PROHIBIT(chemistry .and. num_species > 10, "num_species > 10 for AMDFLang when Case optimization is off") | ||
| @:PROHIBIT(chemistry .and. num_species > 20, "num_species > 20 for AMDFLang when Case optimization is off") |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1852 +/- ##
==========================================
- Coverage 61.26% 61.26% -0.01%
==========================================
Files 84 84
Lines 22330 22331 +1
Branches 3265 3265
==========================================
Hits 13680 13680
- Misses 6207 6208 +1
Partials 2443 2443 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…n/mfc into fix/amd-species-bound
Lines of Code
|
Closes #1848.
When case optimization is off, the
USING_AMDguard substitutes a literal extent fordimension(num_species). That literal was ten, across 21 declarations in nine files, withm_checker_common.fppstating the matching constraint:So a mechanism above ten species cannot be built that way, and the check meant to explain it never runs.
m_thermochem.f90is generated per mechanism with concrete bounds, so an eleven-species mechanism is a compile error long before the runtime check, and what the reader sees isrepeated across
m_chemistry.fpp,m_cbc.fpp,m_riemann_state.fpp,m_ibm.fppand the three Riemann solvers, rather than the sentence describing the limit. Hit while reviewing #1821, whose carbon surface mechanism has eleven species.This raises the bound to twenty and updates the check to match, so the stated limit and the real one agree.
The reductions had to move with it
Raising the literal alone is wrong, and the first commit here was. These arrays are kernel-private and only their first
num_specieselements are ever written, so a whole-array reduction over one of them is correct only while the literal equalsnum_species— which is what ten happened to be for the mechanisms in the test suite. At twenty,sum(Xs(:)/(Gamma_i(:) - 1.0_wp))sums ten elements of uninitialised device scratch. Zero-filled scratch gives the right answer; anything else gives a quietly wronggamma, or a divide by zero ifCp_ilands on one.The second commit pins every such reduction to
1:num_species:m_cbc.fpp—Gamma_iand thegammasum on thegamma_method = 1pathm_riemann_state.fpp—h_avg_2,Yi_avg, the fourCp_avg/Cv_avgsums,Phi_avgandc_sum_Yi_Phiin the reacting Roe averageThat removes the hidden dependency rather than moving it, and is the right form regardless of what the literal is. The three Riemann solvers'
gamma_L/gamma_Rsums were already sliced. Everything else touching these arrays indexes elementwise over1, num_species, and the generatedm_thermochemdummies are explicit-shape at the mechanism's own count, so a longer actual argument never reaches the padding.Cost
Measured on
C4EB58A8(2D reacting mixing layer), generic chemistry GPU build, amdflang OpenMP offload on MI210. Numbers are the 5-equation pure-fluid HLLC kernel, which carries eleven of these arrays:About two percent. Occupancy does not change: waves per SIMD is
floor(512 / roundup(vgpr, 8)), and 260 and 326 both give one wave, so the extra pressure costs spill traffic rather than a wave. One run each, so read the timing as "no large penalty" rather than an exact figure. A case where HLLC dominates would show more than this one does.Note
rhoYksinm_variables_conversion.fppspelled the same bound as(1:10)and is included here.Raising the literal also surfaced three latent shape mismatches in #1821's new code, where surrounding code assumed the padded array was exactly
num_specieslong. The bound is not only a ceiling; code near it depends on the padding happening to match. That is worth keeping in mind for the alternative in #1848 of retiring these guards once the amdgpu bug they work around is fixed.Testing
All sixteen
Chemistrytests pass on amdflang OpenMP offload on MI210, the configuration the guard applies to. That set covers the paths the slicing fix touches: bothReacting Roe Averagecases, bothRiemann Solver 1/2 -> Gammapairs, and the two isothermal-wall CBC cases. Note that passing tests are not evidence the unsliced version was broken — zero-filled scratch reads as a correct answer, which is the whole hazard. Case-optimized builds are unaffected either way: the guard isnot MFC_CASE_OPTIMIZATION and USING_AMD, so those already takedimension(num_species).