Repository navigation
Conversation
HIP_ARCH was never used: enable_language(HIP) sets CMAKE_HIP_ARCHITECTURES to the GPU of the build machine before archs.cmake reads it, and hip::device compiled the C++ sources for GPU_TARGETS, also detected. HIP_ARCH now seeds CMAKE_HIP_ARCHITECTURES (an explicit value still wins), GPU_TARGETS follows it, and the default is gfx1200;gfx1201 (Navi 44 and Navi 48 can't load each other's code objects). attention_wave.h adds ATTN_WAVE_SIZE, attnShflXor and attnRequireWave32 for porting the attention DPPs to HIP. ROCm's clang no longer exposes the wave size at compile time, so a wave64 build traps at run time instead of failing the build, unless the compiler still defines __AMDGCN_WAVEFRONT_SIZE__. Nothing uses them yet; utest_attention_wave tests them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014d2rht2WMSopuodhtXYZzx
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reconfiguration can overwrite an explicit architecture selection and build binaries for the wrong GPU.
1 open issue
What changed in this PR
Prepares FKL’s attention code for RDNA4 by updating HIP target selection and adding standalone wave32 helpers.
Changes:
- Defaults HIP builds to gfx1200 and gfx1201 and aligns C++ and HIP targets.
- Adds wave32 validation and shuffle helpers with GPU tests.
- Documents architecture selection.
| File | Description |
|---|---|
utests/algorithm/attention/utest_attention_wave.h |
Tests reductions and exchanges within two waves. |
include/fused_kernel/algorithms/attention/attention_wave.h |
Adds wave32 guards and shuffle helpers. |
cmake/libs/hip/hip.cmake |
Aligns HIP package GPU targets. |
cmake/libs/hip/hip_init.cmake |
Seeds architectures before enabling HIP. |
cmake/libs/hip/archs.cmake |
Simplifies target architecture assignment. |
AGENTS.md |
Documents HIP architecture options. |
Review effort: Balanced
albertandaluz
force-pushed
the
rdna4/phase-1-hip-targets
branch
from
October 18, 2026 11:34
007283e to
1cf881b
Compare
…rst configure Follows 1cf881b: the comment in hip_init.cmake and AGENTS.md still said HIP_ARCH defaulted to gfx1200;gfx1201 and was re-applied on reconfigure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014d2rht2WMSopuodhtXYZzx
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.

Phase 1 of the RDNA4 (gfx12) port of
algorithms/attention/.Changes
HIP architectures (
cmake/libs/hip/)HIP_ARCHdidn't do anything onmain.enable_language(HIP)runs beforearchs.cmakeand always setsCMAKE_HIP_ARCHITECTURESto the build machine's GPU, soset_target_hip_arch_flagsalways took that branch. On top of that,hip::devicecompiled each target's C++ sources (tests/main.cpp) forGPU_TARGETS, which hip-config also detects. Checked onmain:-DHIP_ARCH=gfx1201still produced--offload-arch=gfx1200on all 165 compile lines.HIP_ARCHseedsCMAKE_HIP_ARCHITECTURESbeforeenable_language(HIP), andGPU_TARGETSfollowsCMAKE_HIP_ARCHITECTURES, somain.cppgets the same arches as the HIP sources.-DHIP_ARCH="gfx1200;gfx1201"builds for Navi 44 (RX 9060) and Navi 48 (RX 9070/XT), which can't load each other's code objects.HIP_ARCHapplies on the first configure of a build directory, and an explicit-DCMAKE_HIP_ARCHITECTURESwins over it.gfx12-genericalso works throughCMAKE_HIP_ARCHITECTURES.HIP_ARCH=gfx1200;gfx1201→ both, a reconfigure with anotherHIP_ARCHkeeps the first value, explicitCMAKE_HIP_ARCHITECTURES=gfx12-genericwins.Wave size (
attention/attention_wave.h, new; nothing includes it yet)ATTN_WAVE_SIZE = 32,attnShflXor(value, laneMask)(__shfl_xor_sync(0xffffffffu, …)on CUDA,__shfl_xor(…, 32)on HIP), andattnRequireWave32().__AMDGCN_WAVEFRONT_SIZE__, and neitherwarpSizenor__builtin_amdgcn_wavefrontsize()is a constant expression, so a wave64 compile can't be rejected at build time. The header has a#errorfor compilers that still define the macro. Otherwise kernels callattnRequireWave32(), which folds to nothing at -O3 in wave32 and becomes ones_trapin wave64. HIP's__shfl_xor_syncwould need a 64-bit mask, so the HIP path uses__shfl_xor.utests/algorithm/attention/utest_attention_wave.h: 2 waves per block, butterfly sum plus a single xor exchange (lanes must not cross waves). It gets_hip,_cuand_cpptargets; the_cpptarget only prints that there's nothing to test on the CPU.AGENTS.md: theHIP_ARCHdescription is updated.Verified on the RX 9060 (gfx1200, ROCm 10.1)
main→ 48/48 on this branch (the same 47 plusutest_attention_wave_hip), built for gfx1200;gfx1201. Same unique warnings asmain.-DHIP_ARCH=gfx1201: full build succeeds (it can't run here).-DCMAKE_HIP_FLAGS=-mwavefrontsize64): the test traps on the GPU (hardware exception, exit 134) instead of computing wrong results.Only CUDA CI can verify
utest_attention_wave_cu(nvcc build and run on the A2000) andutest_attention_wave_cpp(g++/clang/MSVC). No existing CUDA-visible code changed: the only CUDA-visible additions are the new unused header and this test.clang-formatisn't installed on this machine, so the new files are formatted by hand to.clang-format(4 spaces, at most 120 columns).🤖 Generated with Claude Code
https://claude.ai/code/session_014d2rht2WMSopuodhtXYZzx