Fix wave-size assumption in EvaluateSplitsKernel (RDNA + mixed-arch HIP builds)#17
Open
hrylx wants to merge 1 commit into
Open
Fix wave-size assumption in EvaluateSplitsKernel (RDNA + mixed-arch HIP builds)#17hrylx wants to merge 1 commit into
hrylx wants to merge 1 commit into
Conversation
release/3.1.1 hardcodes kBlockThreads = 64 for HIP, treating it as synonymous with wave64. RDNA (gfx10/11/12) is wave32, so on RDNA hardware a 64-thread block contains two wavefronts and the warp-scoped reduce/broadcast in EvaluateSplitAgent runs twice independently. The result is silently wrong splits (no crash; the static_assert that would catch this is CUDA-only). On make_classification(10k, 50) accuracy drops from 0.96 to 0.85. Query the wave size at launch via hipDeviceGetAttribute and dispatch to EvaluateSplitsKernel<32> or <64>. Both templates are emitted, so one binary covers wave32 and wave64 including multi-arch fatbins. Same approach as the navi PRs (AMD-Ecosystem#6-AMD-Ecosystem#11) on the v2.0.x line, folded into a single launch via an integral_constant-driven lambda. Those PRs were never ported forward; git merge-base with release/3.1.1 is empty, so 3.1.1 ships with the original wave64-only assumption.
|
Thanks for the PR. I experienced the same behavior on my RX 9070 XT. |
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.
Fix wave-size assumption in EvaluateSplitsKernel (RDNA + mixed-arch HIP builds)
amd_xgboost==3.1.1segfaults ondevice="cuda"on every RDNA-class GPU.Reproduced on RX 9070 XT (gfx1201) under ROCm 7.2.2 in
rocm/dev-ubuntu-24.04.There are two separate problems.
1. The wheel's fatbin is missing RDNA code objects
Build-pipeline issue. Adding gfx1100/gfx1200/gfx1201 to the wheel's
--offload-archset fixes it. This PR's source change is what makes thatmeaningful, so they need to land together.
2. The source assumes wave64 wherever HIP is enabled
src/tree/gpu_hist/evaluate_splits.cu:That's only true for CDNA (gfx9). RDNA (gfx10/11/12) is wave32, so a 64-thread
block on RDNA hardware contains two wavefronts.
EvaluateSplitAgentthen breaks:cub::WarpReducereduces within one wavefront,__shfl_sync(kFullMask, x, 0)broadcasts within one wavefront, and the two wavefronts race into the same
__shared__ best_split. No memory access is OOB, so the kernel does not crash;it just picks the wrong split. Trees are silently wrong. On
make_classification(10k, 50)accuracy drops from 0.96 to 0.85. Thestatic_assert(kBlockSize == WARP_SIZE)that would catch this is#if defined(XGBOOST_USE_CUDA)-guarded.Prior history
Branch
release_2.0.3-rocm_navihas eightfix navi wave32commits by @hliucabetween 2025-03 and 2025-04 (tip
ee5234f2c). They diagnose the same problem anddispatch via a runtime
hipDeviceGetAttributequery.git merge-base ee5234f2c release/3.1.1is empty: those commits never made it forward, so 3.1.1 ships withthe original wave64-only assumption.
Fix
Same approach as the navi patches: runtime-query the wave size and dispatch to
the matching
EvaluateSplitsKernel<32>orEvaluateSplitsKernel<64>. Theduplicated launch block from the navi version is folded into a single
integral_constant-driven lambda, the lazy-init global is gone, and theprintf+exit(-1)is replaced withLOG(FATAL). One source file, no CMakechanges.
Both kernel templates are emitted, so a single binary covers wave32 and wave64.
Multi-arch builds work as expected:
That should let one
amd_xgboostwheel cover both families.Validation (RX 9070 XT, ROCm 7.2.2)
Single-arch (gfx1201) and mixed-arch (gfx942 + gfx1201) builds give the same
results on RDNA hardware. 6/7 of binary, multiclass, regression, deep-tree, and
500-round configurations match CPU train-loss within 5%. The seventh, a wide
500-feature problem, has 13.7% train-loss diff on a single seed; over 8 seeds
with held-out evaluation the test AUC differs from CPU by 0.04%, which is
FP-reduction-order variance.
CDNA (gfx9) is unchanged. The runtime dispatch picks
kBlockThreads = 64ongfx9 hardware, identical to the original code path.
CI
The CDNA matrix cannot catch this class of bug because
block_size == wavefrontalways holds on gfx9. Adding any gfx10/11/12 target would have caught both this
issue and the earlier 2.0 to 3.1 forward-port loss.