make: consistent ROCm targets (rocm-strix-halo / rocm-generic) + portable lib paths (#357, #179)#365
Open
jamesburton wants to merge 1 commit into
Open
make: consistent ROCm targets (rocm-strix-halo / rocm-generic) + portable lib paths (#357, #179)#365jamesburton wants to merge 1 commit into
jamesburton wants to merge 1 commit into
Conversation
…le lib paths Two ROCm build-ergonomics fixes: - antirez#357: name the ROCm targets like the CUDA ones. Adds `rocm-strix-halo` (gfx1151 preset, mirrors `cuda-spark`) and `rocm-generic` (set ROCM_ARCH=gfxNNN, mirrors `cuda-generic`); `strix-halo` and `rocm` stay as back-compat aliases. Help text updated to match. - antirez#179: build on non-/opt ROCm layouts. Add ROCM_PATH (default /opt/rocm), search both lib and lib64 (Fedora etc. use lib64), link the HIP runtime explicitly (-lamdhip64, not always auto-added), and compile host objects -fPIC so hipcc can link them. All additive. Verified target resolution with `make -n` (offload-arch propagation for both targets + aliases). The Linux ROCm compile itself was not re-run here (authored on a Windows HIP box); changes are additive Makefile-only and recipe-preserving. Closes antirez#357. Addresses antirez#179. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Makefile to improve ROCm/HIP build ergonomics and compatibility across different ROCm installation layouts.
Changes:
- Add
ROCM_PATHand update ROCm link flags to searchlib/lib64and explicitly linkamdhip64. - Introduce ROCm targets aligned with CUDA naming (
rocm-strix-halo,rocm-generic) and keep back-compat aliases. - Refresh
helpoutput to reflect the new ROCm target structure and usage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+40
| # Search both lib and lib64 (Fedora and other non-/opt layouts install ROCm libs | ||
| # under lib64) and link the HIP runtime explicitly; some installs don't auto-add | ||
| # -lamdhip64. See issue #179. Override ROCM_PATH for a wheel/non-default install. |
| CUDA_LDLIBS ?= -lm -Xcompiler -pthread -L$(CUDA_HOME)/targets/sbsa-linux/lib -L$(CUDA_HOME)/lib64 -lcudart -lcublas | ||
| HIPCC ?= $(shell command -v hipcc 2>/dev/null || echo /opt/rocm/bin/hipcc) | ||
| ROCM_PATH ?= /opt/rocm | ||
| ROCM_ARCH ?= gfx1151 |
Comment on lines
+116
to
+118
| rocm-strix-halo: ROCM_ARCH := gfx1151 | ||
| rocm-strix-halo rocm-generic: | ||
| $(MAKE) -B ds4 ds4-server ds4-bench ds4-eval ds4-agent ROCM_ARCH="$(ROCM_ARCH)" \ |
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.
Two small ROCm build-ergonomics improvements (Makefile-only, additive).
#357 — name ROCm targets like the CUDA ones
The CUDA side has
cuda-spark/cuda-generic/cuda CUDA_ARCH=, but ROCm only hadstrix-halo+ arocmalias. This adds the parallel names:make rocm-strix-halo— gfx1151 preset (mirrorscuda-spark)make rocm-generic ROCM_ARCH=gfxNNN— any Radeon (mirrorscuda-generic)make rocm ROCM_ARCH=gfxN— explicit archstrix-haloandrocmremain as back-compat aliases; help text updated to match. (Doing this now, while ROCm just landed, churns the fewest users — per the issue.)#179 — build on non-/opt ROCm layouts
Adds
ROCM_PATH(default/opt/rocm), searches bothlibandlib64(Fedora and other distros install ROCm libs underlib64), links the HIP runtime explicitly (-lamdhip64— not always auto-added), and compiles host objects-fPICso hipcc can link them. These codify the workarounds the #179 reporter verified on Fedora F44.Verification
Target resolution checked with
make -n(correct--offload-archforrocm-strix-halo=gfx1151,rocm-generic ROCM_ARCH=gfx1100=gfx1100, and both aliases). The recipes are unchanged apart from the additive flags, so existingstrix-halobuilds are unaffected. Note: authored on a native-Windows HIP box, so the Linux/opt/rocmcompile wasn't re-run here — would appreciate a maintainer sanity-build, especially of the-lamdhip64/lib64 link line on a standard install.Closes #357. Addresses #179.