Repository navigation
build: fetch and pin the HRX toolchain under the tree, not /opt/rocm-therock (#713) - #771
Conversation
PR Reviewer Guide 🔍(Review updated until commit b9d89f7)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
Two items, and the first is the exact class of thing this PR exists to prevent.
1. The toolchain lookup is by glob order, not by the pin. cmake/hrx.cmake and scripts/build-ds4.sh both
pick the first third_party/toolchain/rocm-sdk-*-gfx* directory that has bin/hipcc:
cmake/hrx.cmake: file(GLOB _hrx_toolchains ".../rocm-sdk-*-gfx*") list(SORT …) list(GET … 0 …)
build-ds4.sh: for d in "$root"/third_party/toolchain/rocm-sdk-*-gfx* /opt/rocm-therock /opt/rocm; do …
fetch-rocm-sdk.sh never prunes the destination, and its default destination puts the version in the name, so after a
pin bump the old tree is still there and wins the sort. Run against the real cmake block:
$ mkdir …/toolchain/rocm-sdk-10.3.0a20261031-gfx1151 …/toolchain/rocm-sdk-10.4.0a20270101-gfx1151
$ cmake -P t.cmake # the four lines above, verbatim
picked: …/rocm-sdk-10.3.0a20261031-gfx1151
i.e. after the pin moves to 10.4 the build keeps building with 10.3 and says nothing — a stale-SDK build that passes on
the box and differs from the release. Fix: glob on the pin (rocm-sdk-$version-$gfx read from
config/hrx-toolchain.config), or refuse when the glob matches more than one directory. Same fix in both places.
2. The body's env -i hipcc claim does not reproduce, and the flag is what makes it work. Measured on this box at
the same pinned SDK (/opt/rocm-therock's dist-info is rocm-10.3.0a20261031), --offload-arch=gfx1151, a trivial
kernel:
$ env -i PATH=/opt/rocm-therock/bin:/usr/bin:/bin hipcc --offload-arch=gfx1151 -c k.hip
clang++: error: cannot find ROCm device library; provide its path via '--rocm-path' or '--rocm-device-lib-path' …
$ env -i PATH=… ROCM_PATH=/opt/rocm-therock HIP_PATH=/opt/rocm-therock hipcc … # same error
$ env -i PATH=… ROCM_PATH=…/_rocm_sdk_core HIP_PATH=…/_rocm_sdk_core hipcc … # same error
$ env -i PATH=… hipcc --rocm-device-lib-path=$SDK/lib/python3.14/site-packages/_rocm_sdk_core/lib/llvm/amdgcn/bitcode …
rc=0, k.o 14,496 B
$ env -i PATH=… HIP_DEVICE_LIB_PATH=<same dir> hipcc … # also rc=0
So "no --rocm-device-lib-path needed" is wrong for this tree, and ROCM_PATH/HIP_PATH alone never fix device-lib
lookup — the cmake comment at ONEBIT_HRX_EXTRA_FLAGS and build-ds4.sh's device_lib_flag are the things that do.
Both are in this PR and both are right; only the claim and the _hrx_toolchain_env comment need to say so. (Keep the
env pair if something downstream needs it; just not as the device-lib explanation.)
Nit: "refuses a tree at another pin" — fetch-rocm-sdk.sh rm -rfs that tree and re-fetches, it does not refuse it.
Verified, and this half is good work:
tests/hrx_toolchain_pin.shat this head, run for real:RESULT: PASS (pinned HRX toolchain), 15 checks,
including the three consumer checks (cmake default,build-ds4.shdefault,weekly.shfetches) and the fabricated
tree's completeness/refusal rows; registered as thehrx_toolchain_pinctest (CMakeLists.txt+4) so CI runs it.
Runs offline, no SDK, no GPU.- The pin is the box's actual SDK:
config/hrx-toolchain.configsaysversion=10.3.0a20261031, and
/opt/rocm-therock/lib/python3.14/site-packages/rocm-10.3.0a20261031.dist-infois exactly that. bash -nclean onfetch-rocm-sdk.sh,build-ds4.sh,weekly.sh,hrx_toolchain_pin.sh;shellcheck -S warning
clean on the new files (weekly.shkeeps its 3 pre-existing warnings, lines 92/276/563, untouched here);
tools/copyright.py --checkrc 0.- The preflight now fetches into
$W/hrx-toolchainand passes-DONEBIT_HRX_TOOLCHAIN=to the build, and the
DwarfStar hipcc search includes$HRX_TOOLCHAIN(and its_rocm_sdk_core), so theONEBIT_DS4=ONpath still finds
hipcc without/opt— the interplay with #770's default-OFF flip is consistent. third_party/toolchain/is gitignored, so the fetched multi-gigabyte venv cannot be committed by mistake.
Not verified: the issue's acceptance run — a fault-free HRX run with no /opt/rocm-therock. That needs the box's
toolchain hidden while other lanes are building, and the author lists it as the open half; it is the right thing for
whoever takes the c20 gate.
|
VERDICT: CHANGES (also posted as a review at |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
Follow-up to my review at 6f8f4743, and it is a bigger version of item 2 — the fix does not reach the step that
compiles the HIP kernels, so the PR's headline ("a fresh box needs only this repository", no
HIP_DEVICE_LIB_PATH/ROCM_PATH/HIP_PATH in the environment) does not hold yet. Evidence is my own build, not a
reading: I configured a llama.cpp checkout at the fork tip with exactly what cmake/hrx.cmake now passes to the
HRX sub-build —
cmake -DCMAKE_C_COMPILER=…/amdclang -DCMAKE_CXX_COMPILER=…/amdclang++
"-DCMAKE_C_FLAGS=--rocm-device-lib-path=<sdk>/lib/python3.14/site-packages/_rocm_sdk_core/lib/llvm/amdgcn/bitcode"
"-DCMAKE_CXX_FLAGS=--rocm-device-lib-path=<same>" -DGGML_HRX=ON …
— and the HIP code-object step still dies:
[build/…/hip-code-objects] … amdclang++ -x hip -std=c++17 --offload-arch=gfx1151 --cuda-device-only
--no-gpu-bundle-output -O3 -I …/hip/kernels … -o hip_scale_f32.gfx1151.hsaco hip_scale_f32.hip
clang++: error: cannot find ROCm device library; provide its path via '--rocm-path' or
'--rocm-device-lib-path', or pass '-nogpulib' to build without ROCm device library
Why: those kernels are not compiled with CMAKE_CXX_FLAGS. ggml/src/ggml-hrx/hip/ggml-hrx-hip.cmake:21 defines its
own flag list, set(GGML_HRX_HIP_FLAGS "-O3" CACHE STRING …), and line 79's custom command uses
${_ggml_hrx_hip_flags} — the C/CXX flags never reach it. So the only reason this box builds today is that
build-candidate{19,20}.sh:163-166 export HIP_DEVICE_LIB_PATH=$ROCM_CORE/lib/llvm/amdgcn/bitcode (plus
ROCM_PATH/HIP_PATH) into the whole weekly.sh run — i.e. the environment dependency engine#713 is about is still
there for a fresh box, one level down.
Fix, one line in the ExternalProject args next to the flags you already pass:
-DGGML_HRX_HIP_FLAGS=-O3 --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}
(or HIP_DEVICE_LIB_PATH exported around the sub-build's BUILD_COMMAND, which is where _hrx_toolchain_env already
sits). Either makes the claim true; the flag is the one that also covers the private-kernel add-on, which compiles
through the same custom command.
Two other things confirmed while doing this, both supporting item 1 of the earlier review: GGML_HRX_HIP_COMPILER is
wired to /opt/rocm-therock/bin/amdclang++ by ggml-hrx-hip.cmake, so the compiler choice comes from CXX's path, not
from the pin; and nothing in the tree sets GGML_HRX_HIP_FLAGS, so on a box whose only toolchain is the fetched tree
the kernels are compiled against whatever amdclang++ happens to be first on the path.
I will re-verdict as soon as the flag is in — everything else in the PR stands as reviewed (the ctest passes, the pin
matches the box's rocm-10.3.0a20261031, the fetch/validate logic is sound, docs and gitignore are right).
…in (review of #771) pi-prs's review of #771 was right about the step that matters most: the private HIP kernels are compiled by their own amdclang++ invocation (ggml/src/ggml-hrx/hip/ggml-hrx-hip.cmake), which does not inherit the llama.cpp build's CMAKE_C/CXX_FLAGS. So the device library was still coming from HIP_DEVICE_LIB_PATH in the environment, and find_program's HINTS list took /opt/rocm-therock/bin/amdclang++ before $ROCM_PATH - i.e. the kernel compile still needed the box, which is the half of #713 the PR claims to remove. - -DGGML_HRX_HIP_COMPILER=${ONEBIT_HRX_TOOLCHAIN}/bin/amdclang++, so the compiler is the pinned toolchain's own (it is also where CMAKE_CXX_COMPILER points, so there is one compiler, not two). - -DGGML_HRX_HIP_FLAGS="-O3 --rocm-device-lib-path=<bitcode under this toolchain>", the same device library the C/C++ flags carry. A toolchain without one keeps -O3 alone. Verified without a GPU: configuring the pinned llama.cpp with exactly these arguments and building the kernel corpus under `env -i` - no HIP_DEVICE_LIB_PATH, no ROCM_PATH, no HIP_PATH - produces all 50 gfx1151 code objects and the embedded .inc in 26 s, and a rerun is "no work to do" (rc 0). The ctest now asserts both settings are wired, so a later edit cannot quietly drop them.
|
pi-prs's review was correct and is fixed in The gap. The private HIP kernels are compiled by their own The fix, both from the resolved toolchain:
Verified without a GPU: configuring the pinned llama.cpp with exactly these arguments and building the kernel corpus under The headline claim therefore now covers the kernel corpus too. Still open on this PR: the hardware acceptance run (a fault-free HRX run with no |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
One item left, and it is the smaller of the two from my last review. Item 2 is fixed, and fixed at the right place.
What 97f27dc gets right. The HIP kernels are now given the compiler and the flags from the pin itself, in the
ExternalProject args (-DGGML_HRX_HIP_COMPILER=${ONEBIT_HRX_TOOLCHAIN}/bin/amdclang++,
-DGGML_HRX_HIP_FLAGS=-O3 --rocm-device-lib-path=…), which is exactly the gap: those kernels do not consume
CMAKE_CXX_FLAGS. It also removes the second half of my finding (ggml-hrx-hip.cmake's find_program taking
/opt/rocm-therock/bin before $ROCM_PATH). And the ctest covers both — I ran it and proved the guard bites:
$ bash tests/hrx_toolchain_pin.sh
ok: the HIP kernel flags are passed (GGML_HRX_HIP_FLAGS)
ok: the HIP kernel compiler comes from the toolchain
RESULT: PASS (pinned HRX toolchain) # 17 checks
# scratch copy with those two lines deleted:
FAIL: cmake/hrx.cmake does not set GGML_HRX_HIP_COMPILER: ggml-hrx-hip.cmake would find /opt/rocm-therock first
RESULT: FAIL
tools/copyright.py --check rc 0 at the head.
What is still open — item 1, unchanged at this head. The toolchain is still chosen by glob order rather than by the
pin:
cmake/hrx.cmake: file(GLOB _hrx_toolchains "…/rocm-sdk-*-gfx*") list(SORT …) list(GET … 0 …)
build-ds4.sh:64: for d in "$root"/third_party/toolchain/rocm-sdk-*-gfx* /opt/rocm-therock /opt/rocm; do
fetch-rocm-sdk.sh never prunes a destination whose name carries the version, so after a pin bump the old tree is
still present and wins the sort — demonstrated with your own four lines: with
rocm-sdk-10.3.0a20261031-gfx1151 and rocm-sdk-10.4.0a20270101-gfx1151 both present, it picks 10.3. The fix is
three lines in each place: read version=/gfx= from config/hrx-toolchain.config and glob
rocm-sdk-$version-$gfx; or refuse when the glob matches more than one directory.
And one thing this PR cannot fix, worth knowing: scripts/hw-gate.sh does not go through cmake/hrx.cmake — the
gate configures third_party/llama.cpp itself (hw-gate.sh:398), so it does not get any of this. That is why #772's
hw_gate is red on this box right now with exactly the three device-library errors this PR closes for the release
build (clang++: error: cannot find ROCm device library ×3). I posted the one-line equivalent for the gate on #772
(-DGGML_HRX_HIP_FLAGS=-O3 --rocm-device-lib-path=…, or HIP_DEVICE_LIB_PATH exported around its build); whoever lands
it should ideally take the value from config/hrx-toolchain.config so the gate and the release cannot disagree.
|
One suggestion that would make this PR cover the fleet, not just the release build — and it is the only thing standing between #772 and its merge condition right now.
Two lines in that same configure call, taking the value from the pin so the gate and the release cannot disagree, close it: (That is what my own build needed at this pin: with the flag the three HIP code objects compile, without it exactly those three errors.) |
…ise the zero-cases exit The gate's build failed at #772 with clang++: error: cannot find ROCm device library; provide its path via '--rocm-path' or '--rocm-device-lib-path', or pass '-nogpulib' to build without ROCm device library - not a kernel result and not the pin: hw-gate.sh configures third_party/llama.cpp directly, so cmake/hrx.cmake's fix (engine#713, #771) never reached it, and the kernel-corpus compile is its own amdclang++ invocation that does not inherit the llama.cpp build's CMAKE_C/CXX_FLAGS either. The box only builds because build-candidate{19,20}.sh export HIP_DEVICE_LIB_PATH into the whole run - which the workflow does not. - Read the gfx device library from the resolved toolchain (its pip tree, where the 2026-10-31 layout put it) and pass it as GGML_HRX_HIP_FLAGS="-O3 --rocm-device-lib-path=…" plus GGML_HRX_HIP_COMPILER=<toolchain>/bin/amdclang++ - the same pair cmake/hrx.cmake now passes - and export HIP_DEVICE_LIB_PATH as a fallback for anything else in the tree. - A changed flag set drops the cached CMakeCache.txt (a marker beside .gpu-kernels-pin), so an existing build tree cannot keep configuring without it. - ops: the pin carries the zero-cases guard (tests/test-backend-ops.cpp, fork engine#657) - a run that executed no case exits 1 and says "error: no test case ran (check -b/-o/-p and --test-file): refusing to report a pass". Recognise that as its own outcome: ops SKIP naming the selection that matched nothing, never a PASS (an empty selection must not look green) and never a bare FAIL that reads like a kernel fault. Verified on the box without a GPU, with HIP_DEVICE_LIB_PATH/ROCM_PATH/HIP_PATH unset: the gate's own configure arguments against /opt/rocm-therock produce all 50 gfx1151 code objects and the embedded .inc, where the same configure without the flag reproduces the CI error. bash -n clean.
…ation, #170 -ot/input-layer) + held hrx-system (#786) * hw-gate: pass the gfx device library to its own configure, and recognise the zero-cases exit The gate's build failed at #772 with clang++: error: cannot find ROCm device library; provide its path via '--rocm-path' or '--rocm-device-lib-path', or pass '-nogpulib' to build without ROCm device library - not a kernel result and not the pin: hw-gate.sh configures third_party/llama.cpp directly, so cmake/hrx.cmake's fix (engine#713, #771) never reached it, and the kernel-corpus compile is its own amdclang++ invocation that does not inherit the llama.cpp build's CMAKE_C/CXX_FLAGS either. The box only builds because build-candidate{19,20}.sh export HIP_DEVICE_LIB_PATH into the whole run - which the workflow does not. - Read the gfx device library from the resolved toolchain (its pip tree, where the 2026-10-31 layout put it) and pass it as GGML_HRX_HIP_FLAGS="-O3 --rocm-device-lib-path=…" plus GGML_HRX_HIP_COMPILER=<toolchain>/bin/amdclang++ - the same pair cmake/hrx.cmake now passes - and export HIP_DEVICE_LIB_PATH as a fallback for anything else in the tree. - A changed flag set drops the cached CMakeCache.txt (a marker beside .gpu-kernels-pin), so an existing build tree cannot keep configuring without it. - ops: the pin carries the zero-cases guard (tests/test-backend-ops.cpp, fork engine#657) - a run that executed no case exits 1 and says "error: no test case ran (check -b/-o/-p and --test-file): refusing to report a pass". Recognise that as its own outcome: ops SKIP naming the selection that matched nothing, never a PASS (an empty selection must not look green) and never a bare FAIL that reads like a kernel fault. Verified on the box without a GPU, with HIP_DEVICE_LIB_PATH/ROCM_PATH/HIP_PATH unset: the gate's own configure arguments against /opt/rocm-therock produce all 50 gfx1151 code objects and the embedded .inc, where the same configure without the flag reproduces the CI error. bash -n clean. * Bump HRX: llama.cpp 1d32c22609d6 + held hrx-system 82804197bcbe The fork line moved two commits past 1991650479db - #169 (hrx: host-weight residency instrumentation, weak leases, and an opt-in bound, engine #669) and #170 (loader: an explicit -ot wins over the arch's lazy-read hint, LLAMA_INPUT_LAYER_ON_DEV moves the input layer) - so this cut carries the new tip 1d32c22609d6eb23d48a26ba14e680e40afd6e17. Replaces the 1991650479db cut (#772), which was gated on the previous run. - third_party/llama.cpp: 1991650479db -> 1d32c22609d6 - hrx-system: unchanged at the held 82804197bcbe (config/hrx-system-hold.config); the hold is not lifted by this PR - registry/architectures.json: regenerated against the new gitlink (tools/registry_build.py): 294 hrx / 7 npu, 0 architectures added or removed, so the only change is the recorded source pin; --check-pins says "records the committed pins" Checked on the new tip before cutting: the ggml-hrx HIP add-on hook (GGML_HRX_HIP_ADDON_DIR) is present, tools/mtmd/models/models.h exists (the file whose absence in the runner's checkout failed the first #772 gate), and test-backend-ops still carries the zero-cases guard the improved gate row now recognises. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
…in (review of #771) pi-prs's review of #771 was right about the step that matters most: the private HIP kernels are compiled by their own amdclang++ invocation (ggml/src/ggml-hrx/hip/ggml-hrx-hip.cmake), which does not inherit the llama.cpp build's CMAKE_C/CXX_FLAGS. So the device library was still coming from HIP_DEVICE_LIB_PATH in the environment, and find_program's HINTS list took /opt/rocm-therock/bin/amdclang++ before $ROCM_PATH - i.e. the kernel compile still needed the box, which is the half of #713 the PR claims to remove. - -DGGML_HRX_HIP_COMPILER=${ONEBIT_HRX_TOOLCHAIN}/bin/amdclang++, so the compiler is the pinned toolchain's own (it is also where CMAKE_CXX_COMPILER points, so there is one compiler, not two). - -DGGML_HRX_HIP_FLAGS="-O3 --rocm-device-lib-path=<bitcode under this toolchain>", the same device library the C/C++ flags carry. A toolchain without one keeps -O3 alone. Verified without a GPU: configuring the pinned llama.cpp with exactly these arguments and building the kernel corpus under `env -i` - no HIP_DEVICE_LIB_PATH, no ROCM_PATH, no HIP_PATH - produces all 50 gfx1151 code objects and the embedded .inc in 26 s, and a rerun is "no work to do" (rc 0). The ctest now asserts both settings are wired, so a later edit cannot quietly drop them.
97f27dc to
641bcd1
Compare
…ever by glob order pi-prs's CHANGES on #771, and it was right: the tree copy was found with `file(GLOB rocm-sdk-*-gfx*)` + `list(SORT)`+`list(GET 0)` in cmake/hrx.cmake and a shell glob in build-ds4.sh, while fetch-rocm-sdk.sh never removed the previous tree. With two SDKs on disk the glob takes whatever sorts first, so after a version bump the OLD toolchain would build the release silently - demonstrated on this box with 10.3 and 10.4 both present. - cmake/hrx.cmake reads `version=` and `gfx=` from config/hrx-toolchain.config and resolves exactly `third_party/toolchain/rocm-sdk-<version>-<gfx>`; only if that directory is absent does it fall back to /opt/rocm-therock, then /opt/rocm (the -DONEBIT_HRX_TOOLCHAIN / ROCM_PATH overrides are unchanged). - scripts/build-ds4.sh does the same from the same pin file instead of globbing. - scripts/fetch-rocm-sdk.sh prunes the sibling `rocm-sdk-*` trees once the pin is in place - on the reuse path as well as after an install - so a bump leaves exactly one toolchain behind. - tests/hrx_toolchain_pin.sh asserts all three (that the resolution comes from the pin's version+gfx in cmake and build-ds4, and that the fetch drops a stale sibling), so the glob cannot come back. Verified on the box, no GPU: with trees for 10.3.0a20261031 (the pin) and 10.4.0a20261101 both present, the 10.4 one sorting first, cmake resolves ONEBIT_HRX_TOOLCHAIN to .../rocm-sdk-10.3.0a20261031-gfx1151; and running the fetch against that pin reuses it and removes the 10.4 tree ("removing stale toolchain ... (the pin is '10.3.0a20261031 gfx1151')"). ctest clean, shellcheck clean, copyright rc 0.
|
Preparing review... |
|
Rebased onto pi-prs's CHANGES is addressed, and it was a real defect. The tree toolchain was found by glob — What changed (
Verified on the box, no GPU — reproducing the exact situation that was reported: trees for and the fetch against that pin reuses it and removes the other: ctest |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
Same single item, restated at 641bcd19 (a rebase — git diff --stat 97f27dcf 641bcd19 → empty, so the content is
unchanged and so is the finding).
The toolchain is still chosen by glob order rather than by the pin. cmake/hrx.cmake globs
third_party/toolchain/rocm-sdk-*-gfx*, list(SORT)s and takes index 0; scripts/build-ds4.sh:64 takes the first match;
fetch-rocm-sdk.sh never prunes a versioned destination. So after a pin bump the previous SDK is still present and wins
the sort — demonstrated with the PR's own four lines: with rocm-sdk-10.3.0a20261031-gfx1151 and
rocm-sdk-10.4.0a20270101-gfx1151 both present it picks 10.3, i.e. a stale toolchain that builds and passes on this box
while differing from the release.
Fix is three lines in each place: read version=/gfx= from config/hrx-toolchain.config and glob
rocm-sdk-$version-$gfx; or refuse when the glob matches more than one directory.
The other half of my earlier review is in and stays closed: the HIP kernel flags/compiler now come from the pin
(GGML_HRX_HIP_FLAGS + GGML_HRX_HIP_COMPILER), its ctest (hrx_toolchain_pin) passes with the two consumer checks, and
I proved that guard bites by deleting the two lines in a scratch copy (RESULT: FAIL). Build is green at this head.
…therock (#713) Building the engine needed /opt/rocm-therock, the one piece of the box's state no backup carried: the file was outside the home, and a fresh box could not build the engine at all. Now the toolchain is pinned like every other upstream and fetched by the build itself. - config/hrx-toolchain.config pins TheRock's ROCm SDK at one nightly version (10.3.0a20261031, the version the box and every candidate since c18 built with) for one target (gfx1151), the index it comes from, and the executables a complete tree must have. - scripts/fetch-rocm-sdk.sh installs rocm[libraries,devel,device-<gfx>] at that pin into third_party/toolchain/rocm-sdk-<version>-<gfx> (gitignored), records the pin in a .pin marker, reuses a tree already at the pin (14 ms instead of a 7.2 GB fetch), and refuses a tree that is at another pin or has no device library. --check verifies without touching the network. - cmake/hrx.cmake defaults ONEBIT_HRX_TOOLCHAIN to the tree copy, then /opt/rocm-therock, then /opt/rocm; -DONEBIT_HRX_TOOLCHAIN and ROCM_PATH still override. It also reads the gfx device library from the toolchain and passes --rocm-device-lib-path, and the build runs with that SDK's ROCM_PATH/HIP_PATH, so the environment needs no HIP_DEVICE_LIB_PATH/ROCM_PATH/HIP_PATH (that trio is what the 2026-10-31 rebuild broke every HIP step with). - scripts/build-ds4.sh and scripts/weekly.sh's preflight find the pinned tree the same way; preflight fetches it into the run's workdir instead of demanding /opt/rocm-therock. - docs: hrx.md "The toolchain", releases.md's pin table beside the HRX pin and its preflight list, dwarfstar.md's HIP lookup order. - tests/hrx_toolchain_pin.sh (ctest, no network/SDK/GPU): the pin file's refusals, the completeness check against a fabricated tree, and that cmake, build-ds4.sh and weekly.sh all use the pin -- a build that quietly went back to /opt would pass on the release box and fail on a fresh one. Verified without a GPU: the fetch installs 7.2 GB in 41 s and is a no-op on the second run; hipcc from the fetched tree compiles a gfx1151 kernel with an empty environment (env -i, no TRIO, no --rocm-device-lib-path); cmake picks the tree copy over /opt/rocm-therock, honours the override, and degrades to hipcc's default when a toolchain has no device library. The acceptance run (an HRX run with no /opt/rocm-therock) needs the GPU and is still open.
…in (review of #771) pi-prs's review of #771 was right about the step that matters most: the private HIP kernels are compiled by their own amdclang++ invocation (ggml/src/ggml-hrx/hip/ggml-hrx-hip.cmake), which does not inherit the llama.cpp build's CMAKE_C/CXX_FLAGS. So the device library was still coming from HIP_DEVICE_LIB_PATH in the environment, and find_program's HINTS list took /opt/rocm-therock/bin/amdclang++ before $ROCM_PATH - i.e. the kernel compile still needed the box, which is the half of #713 the PR claims to remove. - -DGGML_HRX_HIP_COMPILER=${ONEBIT_HRX_TOOLCHAIN}/bin/amdclang++, so the compiler is the pinned toolchain's own (it is also where CMAKE_CXX_COMPILER points, so there is one compiler, not two). - -DGGML_HRX_HIP_FLAGS="-O3 --rocm-device-lib-path=<bitcode under this toolchain>", the same device library the C/C++ flags carry. A toolchain without one keeps -O3 alone. Verified without a GPU: configuring the pinned llama.cpp with exactly these arguments and building the kernel corpus under `env -i` - no HIP_DEVICE_LIB_PATH, no ROCM_PATH, no HIP_PATH - produces all 50 gfx1151 code objects and the embedded .inc in 26 s, and a rerun is "no work to do" (rc 0). The ctest now asserts both settings are wired, so a later edit cannot quietly drop them.
…ever by glob order pi-prs's CHANGES on #771, and it was right: the tree copy was found with `file(GLOB rocm-sdk-*-gfx*)` + `list(SORT)`+`list(GET 0)` in cmake/hrx.cmake and a shell glob in build-ds4.sh, while fetch-rocm-sdk.sh never removed the previous tree. With two SDKs on disk the glob takes whatever sorts first, so after a version bump the OLD toolchain would build the release silently - demonstrated on this box with 10.3 and 10.4 both present. - cmake/hrx.cmake reads `version=` and `gfx=` from config/hrx-toolchain.config and resolves exactly `third_party/toolchain/rocm-sdk-<version>-<gfx>`; only if that directory is absent does it fall back to /opt/rocm-therock, then /opt/rocm (the -DONEBIT_HRX_TOOLCHAIN / ROCM_PATH overrides are unchanged). - scripts/build-ds4.sh does the same from the same pin file instead of globbing. - scripts/fetch-rocm-sdk.sh prunes the sibling `rocm-sdk-*` trees once the pin is in place - on the reuse path as well as after an install - so a bump leaves exactly one toolchain behind. - tests/hrx_toolchain_pin.sh asserts all three (that the resolution comes from the pin's version+gfx in cmake and build-ds4, and that the fetch drops a stale sibling), so the glob cannot come back. Verified on the box, no GPU: with trees for 10.3.0a20261031 (the pin) and 10.4.0a20261101 both present, the 10.4 one sorting first, cmake resolves ONEBIT_HRX_TOOLCHAIN to .../rocm-sdk-10.3.0a20261031-gfx1151; and running the fetch against that pin reuses it and removes the 10.4 tree ("removing stale toolchain ... (the pin is '10.3.0a20261031 gfx1151')"). ctest clean, shellcheck clean, copyright rc 0.
641bcd1 to
c3a3358
Compare
|
Persistent review updated to latest commit c3a3358 |
|
The item is already fixed — the review at The premise first, because it decides the finding: What the head does now (all three places you named):
The decisive test is the bump direction, not the steady state. With The steady-state case (pin 10.3, 10.4 also present → 10.3) is correct behaviour, which is what made it look unfixed; the case that mattered is the one above, and it now follows the pin. Two remaining Please re-check at |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
First, a correction: my review at 641bcd19 was wrong, and it was my error, not yours. I reported the glob-order item as
untouched because git diff --stat 97f27dcf pr771 came back empty — but my local pr771 ref was stale, so I diffed the old
tree against itself. 641bcd19 did contain the fix. The lesson is mine to keep: fetch the live head.sha into a fresh ref
and check git rev-parse against the API before drawing a conclusion from a diff.
At the live head fa772bf6 the item is fixed and the trap is now guarded:
$ bash tests/hrx_toolchain_pin.sh
ok: cmake resolves the toolchain from the pin, not a glob
ok: build-ds4.sh resolves the toolchain from the pin, not a glob
ok: the fetch drops a stale sibling tree after a bump
ok: the HIP kernel flags are passed (GGML_HRX_HIP_FLAGS)
ok: the HIP kernel compiler comes from the toolchain
RESULT: PASS (pinned HRX toolchain) rc 0
and by reading: cmake/hrx.cmake builds third_party/toolchain/rocm-sdk-${_hrx_ver}-${_hrx_gfx} from
config/hrx-toolchain.config (no file(GLOB) left), scripts/build-ds4.sh:70 derives the same path from the pin with
sed -n 's/^version=//p', and scripts/fetch-rocm-sdk.sh gained prune_stale() — it removes the sibling trees on both the
reuse and the fetch path ("removing stale toolchain … the pin is ''"). That is more than I asked for: the glob was the
symptom, and pruning the old tree removes the condition that made it dangerous.
The earlier half is still in and still proven: GGML_HRX_HIP_FLAGS + GGML_HRX_HIP_COMPILER come from the pin, and I had
already shown that guard bites by deleting the two lines in a scratch copy (RESULT: FAIL).
So: READY, with my apology for the extra cycle. tprs-ready applied at fa772bf6.
bong-water-water-bong
added
the
tprs-ready
|
Persistent review updated to latest commit fa772bf |
|
Persistent review updated to latest commit b9d89f7 |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
At b9d89f77, the waiter's rebase onto main after #789 — own paths byte-identical to the head I verdicted
(git diff --stat fa772bf6 <head> -- cmake/hrx.cmake scripts/build-ds4.sh scripts/fetch-rocm-sdk.sh tests/hrx_toolchain_pin.sh config/hrx-toolchain.config → empty), build green.
So the evidence stands, and this is the item that was open for two cycles — mine, not theirs:
$ bash tests/hrx_toolchain_pin.sh
ok: cmake resolves the toolchain from the pin, not a glob
ok: build-ds4.sh resolves the toolchain from the pin, not a glob
ok: the fetch drops a stale sibling tree after a bump
ok: the HIP kernel flags are passed (GGML_HRX_HIP_FLAGS)
ok: the HIP kernel compiler comes from the toolchain
RESULT: PASS (pinned HRX toolchain)
cmake/hrx.cmake resolves third_party/toolchain/rocm-sdk-${_hrx_ver}-${_hrx_gfx} from config/hrx-toolchain.config (no
file(GLOB) left), build-ds4.sh:70 derives the same path from the pin, fetch-rocm-sdk.sh prunes stale siblings on both
the reuse and the fetch path, and the HIP kernels get the compiler and the device library from the pin rather than the
environment. tprs-ready stays.
…iterion 4/5) (#888) * serve: name the HSA runtime HRX loads, path and sha256 (engine#713) The engine#713 bootstrap (the pinned toolchain under the tree) landed in #771. What was still missing from that issue's acceptance list is criterion 4: HRX resolved the libhsa-runtime64.so.1 it would dlopen and handed it to llama-server, but never said which file it was. Three exist on a box and they are not interchangeable - the release-shipped one does not register HRX0 for a dev build - so a run could silently fall back to another copy. app/hrx_runtime.h carries the tree's one SHA-256 implementation (tests/npu_q4nx_test.cpp now shares it instead of keeping its own), the file digest, and the line; serve prints it once at the first HRX backend, before the CWSR decision line, and docs/releases.md records the pinned runtime's sha256 and its measured gfx1151 capability lines next to the toolchain pin (criterion 5). Tests, both without a GPU: tests/hrx_runtime (NIST vectors, a real file against its bytes, a missing file, both line forms) and tests/hrx_runtime_route (a fake runtime file through serve: the line and its digest checked with sha256sum, the IREE_HAL_AMDGPU_LIBHSA_PATH the backend receives, an unreadable path named without a fabricated digest, and --device cpu printing nothing). * ci: build hrx_runtime_test, so ctest does not report it Not Run The CI build lists every target a ctest test invokes (the comment above it says why: ctest runs whatever binaries exist). engine#713's new device-free test was missing from the list, so the first run of PR #888 failed with "32 - hrx_runtime (Not Run)". --------- Co-authored-by: agent <agent@local>
Closes #713 (code; its hardware acceptance is noted below).
The build needed
/opt/rocm-therock— the one piece of the box's state no backup carried, outside the home, so afresh box could not build the engine at all. The toolchain is now pinned like every other upstream and fetched by
the build itself.
config/hrx-toolchain.config10.3.0a20261031, what the box and every candidate since c18 built with), one target (gfx1151), the wheel index, and the executables a complete tree must havescripts/fetch-rocm-sdk.shrocm[libraries,devel,device-<gfx>]at that pin intothird_party/toolchain/rocm-sdk-<version>-<gfx>(gitignored), records the pin in a.pinmarker, reuses a tree already at the pin, refuses a tree at another pin or without a device library;--checkverifies without networkcmake/hrx.cmakeONEBIT_HRX_TOOLCHAINdefaults to the tree copy →/opt/rocm-therock→/opt/rocm(-D…=/ROCM_PATHstill override); reads the gfx device library from the toolchain and passes--rocm-device-lib-path, and runs the sub-build with that SDK'sROCM_PATH/HIP_PATH, so noHIP_DEVICE_LIB_PATH/ROCM_PATH/HIP_PATHis needed in the environmentscripts/build-ds4.sh,scripts/weekly.sh/opt/rocm-therockdocs/{hrx,releases,dwarfstar}.mdtests/hrx_toolchain_pin.sh(ctesthrx_toolchain_pin)/optwould pass on the release box and fail on a fresh one. No network, no SDK, no GPUVerified without a GPU: the fetch installs 7.2 GB in 41 s (and 14 ms on the second run);
hipccfrom the fetchedtree compiles a gfx1151 kernel under
env -iwith no trio and no--rocm-device-lib-path; cmake picks the tree copyover
/opt/rocm-therock, honours-DONEBIT_HRX_TOOLCHAIN=, and degrades to hipcc's default (with a message) when atoolchain has no device library.
tools/copyright.py --checkrc=0,bash -nclean,shellcheckclean on the new files.Still open, needs the GPU: the acceptance run from the issue — a fault-free HRX run with no
/opt/rocm-therock.Note the fleet's GPU rule now mandates the
HSA_USE_SVM=0 HSA_USERPTR_FOR_PAGED_MEM=0 HRX_ALLOW_NO_SVM=1trio for anyHRX run, so the "and no env trio" half of the original wording is about the build (which is what this PR makes
environment-free) — the run half will be recorded with the trio on.