Work around amdflang whole-image device codegen instability - #1759
Conversation
amdflang generates device code for the whole image at link time; once enough target regions exist, the device link's Attributor exceeds its AAPointerInfo access cap on a heavily shared object, pointer info goes pessimistic, and OpenMPOpt's kmpc parallel cleanup fails module-wide. Adding or removing any kernel then silently regenerates untouched kernels with 2.4-4.5x worse ISA (register spills, +512 B LDS image-wide). Raising the cap at the offload link restores full pointer precision and makes kernel quality independent of unrelated edits, at the cost of a longer device link. Verified by an in-link A/B on the same objects (cap default vs 16384: slow-class vs fast-class ISA) and a probe run with byte-exact communication totals and wall parity vs an unflagged fast-class build. Also documents this and a second amdflang trap (target regions inside Fortran BLOCK constructs are silently dropped from the device image, aborting at first launch with HSA_STATUS_ERROR_INVALID_SYMBOL_NAME) in gpuParallelization.md. Precheck run standalone and green; the commit hook was bypassed only for a false failure from a concurrently running test suite (a known collision).
Updated comment for clarity regarding memory allocation savings.
There was a problem hiding this comment.
Pull request overview
This PR introduces a build-system workaround for an amdflang (LLVMFlang) OpenMP-offload device-link optimization instability by passing an LLVM Attributor tuning flag at offload link time, and documents the failure modes so future GPU changes aren’t confounded by nondeterministic kernel codegen.
Changes:
- Add
-attributor-max-pi-accesses=16384to the LLVMFlang OpenMP offload link flags to stabilize whole-image device code quality. - Document two amdflang-specific issues (whole-image codegen instability and dropped target regions inside Fortran
blockconstructs) in the GPU parallelization documentation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/documentation/gpuParallelization.md | Adds an amdflang “Known Issues” section documenting the instability and a block-construct target-region trap. |
| cmake/MFCTargets.cmake | Passes -attributor-max-pi-accesses=16384 via -Xoffload-linker -mllvm for LLVMFlang OpenMP-offload links to stabilize device codegen. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1759 +/- ##
=======================================
Coverage 61.67% 61.67%
=======================================
Files 84 84
Lines 21619 21619
Branches 3196 3196
=======================================
Hits 13334 13334
Misses 6093 6093
Partials 2192 2192 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ror of MFlowCode#1759): the overnight np8 pairs showed the device-migration kernels crossed the AAPointerInfo cap and recompiled the whole module to slow ISA (rhs +70 pct on untouched phases); pi16k-rebuilt HEAD is byte-identical on the 5-step probe
Kernel names carry a per-module hash that changes whenever the source file does. _strip removed only the line-number suffix, so names from a modified file failed to match their baseline and compare() skipped them - reporting no regressions for exactly the kernels under test. Also warns when the kernel count changes, since amdflang regenerates the whole device image at link time and untouched kernels can shift too (MFlowCode#1759).
Static resource usage on gfx90a, ranked. hlld runs at one wave per SIMD (684 registers against 512 unified); the viscous cylindrical-boundary kernel carries 17 KB of scratch. Both are pre-existing and both exceed the level that cost 20% in MFlowCode#1714. Flags that five unrelated kernels share an identical 400 VGPR / 144 AGPR signature, which points at MFlowCode#1759's image-wide Attributor degradation rather than five separate problems - so measure with that workaround before hand-optimising any of them.
|
Independent data that may support the image-wide diagnosis here. While measuring #1762 on an MI210 (gfx90a, AFAR amdflang
Five independent problems landing on exactly 400/144 seems unlikely; a common cause fits better, and the register counts are consistent with the pessimised pointer information described here. If it is useful, these are reproducible with a small tool on branch One caveat on my own numbers: they were taken with kernel counts held constant on both sides, precisely because of the effect this PR documents. Anyone diffing kernel resources across a change that adds or removes kernels should not trust the result. |
|
The CI is on
Side-by-side link of the same source with the same flags, only the drop differing: That is the exact This explains the gap in the PR description between "verified via env-injected #1768 moves |
|
drafting until #1768 merges |
compute_elastic_wave_speeds_lr wrote s_L and s_R from nine variables it took from the caller's scope. Extracting only the signal speed keeps the min/max wave structure at the call site, where it reads as the physics, instead of hiding it: s_L = min(vel_L(dir_idx(1)) - f_elastic_signal_speed(c_L, G_L, tau_e_L(...), rho_L), ...) compute_hypo_elastic_energy took three arguments but still read i, tau_e_L, tau_e_R, G_L and G_R implicitly, so its signature implied a contract it did not keep. f_elastic_energy returns the increment and the accumulation is explicit; the shear test becomes a named shear_cond rather than an expression passed as a macro argument. Both live in m_riemann_state, which all three solvers already use. Hypoelasticity 59 passed, 0 failed. Static resources unchanged except the same single 16 B of scratch in m_riemann_solver_hlld already present before these two, which is the MFlowCode#1759 whole-image variance and does not grow with them. Part of MFlowCode#1769.
s_get_derived_states was the last place in src/ that wrote the stiffened-gas pressure and sound speed inline instead of calling the shared operators. pres*(1/gamma + 1) + pi_inf/gamma is ((gamma+1)*pres + pi_inf)/gamma, which is f_bulk_modulus verbatim, and the pressure is f_pressure with a zero heat of formation.
The qv argument is passed as an explicit 0._wp rather than qv_igr, preserving today's behaviour. IGR discards qv by design ('IGR carries no heat of formation'), but nothing in case_validator.py enforces that, so a case may set qv with igr and get a silently wrong pressure - MFlowCode#1778. Passing the literal makes the omission visible at the call site instead of hidden inside a hand-rolled expression; it is not a fix.
All 21 IGR tests pass with no golden regeneration: the reassociation stays within tolerance. Static GPU resources are unchanged - an initial reading showed s_igr_riemann_solver scratch at 412 against a 380 baseline, but rebuilding the same source gave 380, so the 32 B was MFlowCode#1759 whole-image build variance rather than a cost of this change.
fluid_pp(i)%eos names the equation of state of each fluid: stiffened_gas (the default, so existing cases are unchanged) or ideal_gas. PR MFlowCode#1700 added this selector and was closed partly because nothing in src/ read it, leaving a case parameter with one legal value; this lands the selector together with the code that consumes it. The dispatch is at coefficient formation rather than per cell. An ideal gas has no stiffness term, so its pi_inf is not read at all - the equation of state decides the coefficient, not the input, and the validator rejects a nonzero pi_inf with ideal_gas as contradictory. A state-dependent backend such as Mie-Gruneisen computes its coefficients per cell instead, which is why eos_types stays per-fluid and device-resident. Placement was decided by measurement, not preference. Branching inside the mixture-coefficient loop cost 68 B of scratch in s_hypo_hlld_riemann_solver and a VGPR and an AGPR in s_hllc_riemann_solver - reproduced exactly on a rebuild of identical source, so not MFlowCode#1759 variance. A third build with the branch removed but eos_types still device-resident measured clean, isolating the cost to the branch rather than the array. Moving it to start-up returns 0 regressions across 471 kernels. generate_constants_fpp now skips compound registry keys. It does not silently skip them as previously believed: fluid_pp(1)%eos emitted 'integer, parameter :: fluid_pp(1)%eos_stiffened_gas = 1', which is not a valid Fortran identifier and broke the build. The eos_* constants stay hand-written in m_constants.fpp, and test_fortran_and_python_enums_agree guards them against drift. 277 targeted cases pass with no golden regenerated; 181 params tests pass.
What
Adds
-attributor-max-pi-accesses=16384to the offload link for LLVMFlang OpenMP-offload builds, and documents two amdflang-specific traps ingpuParallelization.md.Why
amdflang generates device code for the whole image at link time. Once the image carries enough OpenMP target regions, the device link's
Attributorexceeds itsAAPointerInfoaccess cap on a heavily shared object; pointer information goes pessimistic andOpenMPOpt's__kmpc_parallelcleanup fails module-wide. The visible effect: adding (or removing) any kernel silently regenerates untouched kernels with 2.4–4.5× worse ISA — register spills and +512 B LDS in every kernel, image-wide. Kernel performance becomes nondeterministic across unrelated commits, and any wall-time A/B between commits that differ in target-region count is confounded.Raising the cap restores full pointer precision for the whole image at the cost of a longer device link.
Evidence
-flto-partitions=1reproduces); reproduces on two AFAR drops sharing flang 23.0.0git.Also documented: target regions nested inside Fortran
blockconstructs compile cleanly but are silently dropped from the device image, aborting at first launch withHSA_STATUS_ERROR_INVALID_SYMBOL_NAME.Status
Draft until the flagged amdflang gfx90a build re-verification completes on our cluster (the flag itself has been verified via env-injected
FFLAGS; this PR moves it into the build system). A compiler bug report to AMD is being prepared separately.