Restore HIP feature parity with CUDA; make hipify.sh maintainable - #221
Open
susilehtola wants to merge 1 commit into
Open
Restore HIP feature parity with CUDA; make hipify.sh maintainable#221susilehtola wants to merge 1 commit into
susilehtola wants to merge 1 commit into
Conversation
The HIP backend has not built since mGGA, FXC contraction, EXC gradients
and sn-LinK screening landed on the CUDA side. scheme1_base.cxx is
compiled for both backends (device/CMakeLists.txt:16, before the
conditional cuda/ and hip/ subdirectories) and calls zmat_*_fxc,
increment_exc_grad_*, exx_ek_screening_bfn_stats and the shell-to-task
collocation, none of which had HIP definitions -- so any
-DGAUXC_ENABLE_HIP=ON build fails to link. The HIP files that did exist
were also stale: zmat_vxc.hip still had the pre-UKS/GKS kernel names,
and uvvars.hip had no mGGA path at all.
Root cause is that hipify.sh covered only 8 of the 24 current CUDA
kernel sources and was never updated. This rewrites it to cover all of
them, and replaces the hipify-perl invocations with explicit sed rules
so regeneration is reproducible without a ROCm installation and the
non-mechanical decisions are documented in one place:
* warp/wavefront size comes from GauXC::{cuda,hip}::warp_size (32 vs
64), so launch geometry adapts; no literal 32s were introduced.
* __syncwarp() has no HIP equivalent (lockstep wavefronts on AMD) and
is commented out, the convention the 2022 port already used here.
* __shfl_*_sync(mask, ...) -> __shfl_*(...) (HIP takes no mask).
* cub -> hipcub.
* CUDA's two-tier shared-memory limit has no AMD counterpart: both
cudaDevAttrMaxSharedMemoryPerBlock{,Optin} map to
hipDeviceAttributeMaxSharedMemoryPerBlock, leaving the overflow
guard intact and the opt-in branch a no-op.
* __stcs() and its inline-PTX fallback in pack_submat are lowered to a
plain store; CUDART_VERSION is undefined under HIP, so the
preprocessor would otherwise select the PTX branch.
* CUTLASS is deliberately not translated (CUDA-only dependent option).
Regenerated all 23 kernel files and added zmat_fxc, increment_exc_grad
and exx_ek_screening_bfn_stats to the HIP CMakeLists. Every kernel
symbol scheme1_base.cxx references now has a HIP definition, and no
compile-breaking CUDA token remains in the generated sources.
NOT COMPILE-TESTED: no ROCm or AMD hardware was available. Before this
is trusted for numerical results, it needs a build with hipcc and a
review of any intra-wavefront reduction whose trip count is a literal
rather than derived from hip::warp_size (see CHECK_WAVEFRONT in
hipify.sh).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0135evJ9zgNL1y8U6T9bQ3UT
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.
Problem
The HIP backend does not build.
scheme1_base.cxxis compiled for both backends — it is listed insrc/xc_integrator/local_work_driver/device/CMakeLists.txt:16, before the conditionalcuda/andhip/subdirectories — and it callszmat_*_fxc,increment_exc_grad_*,exx_ek_screening_bfn_statsand the shell-to-task collocation. None of those had HIP definitions, so any-DGAUXC_ENABLE_HIP=ONbuild fails to link.The HIP sources that did exist were stale as well:
kernels/zmat_vxc.hipstill had the pre-UKS/GKS kernel names (zmat_lda_vxc_kernelrather thanzmat_lda_vxc_{rks,uks,gks}_kernel);kernels/uvvars.hiphad no mGGA path at all (zerotaureferences against 19 on the CUDA side);collocation_shell_to_task_kernels.hppand the splituvvars_{lda,gga,mgga}.hppheaders were absent.In effect the HIP backend has been frozen at roughly its 2022 LDA/GGA-VXC feature level while mGGA, FXC contraction, EXC gradients and sn-LinK screening were added on the CUDA side.
Root cause
hip/hipify.shcovered only 8 of the 24 current files undercuda/kernels/and was never updated as kernels were added.What this PR does
Rewrites
hipify.shto cover all current CUDA kernel sources, regenerates the HIP tree from it, and addszmat_fxc,increment_exc_gradandexx_ek_screening_bfn_statstohip/CMakeLists.txt.The
hipify-perlinvocations are replaced with explicitsedrules. Two reasons: regeneration then works on machines without a ROCm installation, and the handful of decisions that are not mechanical end up documented in one place rather than in a maintainer's head:GauXC::cuda::warp_size→GauXC::hip::warp_size(32 vs 64), so launch geometry adapts. No literal 32s were introduced.__syncwarp()has no HIP equivalent (wavefronts are lockstep on AMD) and is commented out — the convention the existing 2022-era files in this directory already used.__shfl_*_sync(mask, …)→__shfl_*(…), since HIP shuffles take no mask argument.cub→hipcub(cub::DeviceScanin the EXX screening kernel).cudaDevAttrMaxSharedMemoryPerBlockand…Optinmap tohipDeviceAttributeMaxSharedMemoryPerBlock. The overflow guard inexx_ek_screeningis preserved; the opt-in branch becomes a no-op.__stcs()and its inline-PTX fallback inpack_submatare lowered to a plain store.CUDART_VERSIONis undefined under HIP, so the preprocessor would otherwise select the PTX branch and fail to compile. The cache hint is an optimization, not semantics.GAUXC_ENABLE_CUTLASSis already a CUDA-only dependent option.Verification
Static checks only (see caveat):
scheme1_base.cxxnow resolves against the HIP tree;hip_aos_scheme1{,_data}.cxxwere checked to have the same method sets as their CUDA counterparts (the line-count difference is CUTLASS-only code), so they did not need regeneration.Caveat — please read before merging
This has not been compiled. I have neither a ROCm installation nor AMD hardware, so the changes have never been through
hipcc. Expect a round of compile fixes.Beyond compilation, the one correctness risk worth a reviewer's eyes is wavefront width: nothing hardcodes 32 and launch geometry derives from
hip::warp_size, but any intra-wavefront reduction whose trip count is written as a literal rather than derived fromwarp_sizeshould be checked before the numbers are trusted on AMD.hipify.shprints aCHECK_WAVEFRONTreminder to that effect.I am happy to iterate on this if someone with an AMD GPU can run the test suite, or to split it into "script + build wiring" and "regenerated kernels" commits if that is easier to review.