Skip to content

hrx: the device library goes to the HIP kernel compile only, not the host C/C++ flags (Refs #1539) - #1563

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/hrx-host-device-lib
Nov 8, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/hrx-host-device-lib

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What is wrong

cmake/hrx.cmake appends --rocm-device-lib-path=<bitcode> to ONEBIT_HRX_EXTRA_FLAGS, which becomes llama_hrx's CMAKE_C_FLAGS and CMAKE_CXX_FLAGS, so the flag reaches every host object of the hrx-system deps build. That build compiles with -Werror. On a host C file the flag has nothing to link, so clang rejects it:

FAILED: runtime/src/iree/async/CMakeFiles/iree_async_async.dir/address.c.o
clang: error: argument unused during compilation: '--rocm-device-lib-path=/opt/rocm-therock/…/amdgcn/bitcode' [-Werror,-Wunused-command-line-argument]

Same compiler (AMD clang 24.0.0git, unchanged since 10-31), the same deps CMakeCache (IREE_ENABLE_WERROR_FLAG=ON), and the same compile command as the bench tree. Running that one compile by hand fails, with or without ccache.

The change

  • cmake/hrx.cmake: the device library is no longer appended to the host flags. The HIP kernels are compiled by their own amdclang++ -x hip --offload-arch=gfx1151 step and already get it from GGML_HRX_HIP_FLAGS (the build: fetch and pin the HRX toolchain under the tree, not /opt/rocm-therock (#713) #771 review), which is unchanged.
  • tests/hrx_toolchain_pin.sh: a new row fails if the flag goes back into ONEBIT_HRX_EXTRA_FLAGS.
  • docs/hrx.md: one sentence saying where the flag goes and why.

Evidence

arm result
main 5481b5d6, cold build FAILED at iree_async_async (-Werror,-Wunused-command-line-argument)
this branch, same build dir reconfigured (deps rebuilt, since the C flags changed) rc 0: 1bit, llama-server, libggml-hrx.so.0.18.0, libhrx.so, libloomc.so built
deps compile commands that carry the flag 0 of 2313 (was every one)
HIP kernel compile still -x hip --offload-arch=gfx1151 … --rocm-device-lib-path=<bitcode> (via GGML_HRX_HIP_FLAGS)
tests/hrx_toolchain_pin.sh PASS. The new row's pattern matches main's hrx.cmake, so the row would be red there.

The build was CPU only, under thermal-run and mem-guard. No device run is needed: the change only touches compile flags of host objects.

🤖 Generated with Claude Code

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at c9cedd4

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at c9cedd403010f04a976578c6ea75f80b1382cfb2

The flag now reaches exactly one compile, which is checkable in the file:

cmake/hrx.cmake:84    set(ONEBIT_HRX_EXTRA_FLAGS "")                        <- no device-lib flag
cmake/hrx.cmake:176   string(APPEND _hrx_hip_flags " --rocm-device-lib-path=…")
cmake/hrx.cmake:180   set(ONEBIT_HRX_HIP_FLAGS "${_hrx_hip_flags}" …)
cmake/hrx.cmake:230   "-DGGML_HRX_HIP_FLAGS=${ONEBIT_HRX_HIP_FLAGS}"

and the reason is a real cold-build failure, with the CI runs named: "the hrx-system deps build compiles with -Werror, and a host C file has no device code to link, so clang rejects the flag as unused (-Wunused-command-line-argument). A cold HRX build failed on it here, and so did hrx-build whenever the deps rebuilt (runs 42524820160, 42526921955)."

That is the right granularity of explanation for a build-flag bug: which flag, which compile it must not reach, the diagnostic the compiler gives, and the two runs where it bit. It also matters for a reason the diff implies but the comment could say explicitly next time: this is a cold-build failure, so a resume of an existing build dir never sees it — the exact shape that makes a flag bug survive for weeks and then fail the one rebuild after a deps change.

cmake/hrx.cmake still parses, and the # block above _hrx_hip_flags now carries the same warning where the next person will edit it.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY once rebased — verified in the source it edits, and it removes only the leaking copy.

What I checked, because "drop a flag" is only safe if the flag is still delivered somewhere:

  • cmake/hrx.cmake:173 already appends --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB} to _hrx_hip_flags, which becomes ONEBIT_HRX_HIP_FLAGS (:177) and reaches the HIP kernel compile (:227). So the device library is still passed where device code is compiled — no feature is lost.
  • The line this PR deletes is :162, appending to ONEBIT_HRX_EXTRA_FLAGS, and :229/:230 pass exactly that into -DCMAKE_C_FLAGS / -DCMAKE_CXX_FLAGS. So the diagnosis is right in both directions: the flag did reach every host object of the -Werror deps build, and removing it touches nothing else.
  • The regression test is the right shape: grep -Eq 'APPEND ONEBIT_HRX_EXTRA_FLAGS[^)]*rocm-device-lib-path' reds on reintroduction and is anchored to the offending statement rather than to the string appearing anywhere (which is what keeps :173 legal). It also matches the removed statement as written on one line, so it is not a vacuous guard.
  • Docs change matches the code (GGML_HRX_HIP_FLAGS, host objects, -Werror).

CI: Analyze ×5 pass; hrx_build pending; build fails on copyright notice duplicated … os/v3/puppy/base-modes-keep — inherited from your base, not this diff: main fixed it in 1571366 (#1560) and python3 tools/copyright.py --check is rc=0 on a main-based tree. A rebase should clear it. I would not spend a cold build proving this locally when hrx_build on a rebased head does it better; say the word if you want me to run one on this box.

pr_agent failing in 12 s is the same fleet-wide observation as on #1568, not this PR.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

Preparing review...

…host C/C++ flags (Refs #1539)

cmake/hrx.cmake appended --rocm-device-lib-path to ONEBIT_HRX_EXTRA_FLAGS, which becomes
llama_hrx's CMAKE_C_FLAGS and CMAKE_CXX_FLAGS and so reaches every host object of the hrx-system
deps build. That build compiles with -Werror, and on a host C file clang rejects the flag as
unused (-Wunused-command-line-argument), so a cold HRX build of main fails in
iree_async_async, and so does hrx-build whenever the deps rebuild (runs 42524820160, 42526921955).

The HIP kernels are compiled by their own amdclang++ step and already get the device library from
GGML_HRX_HIP_FLAGS (#771's review), so the host copy is dropped. tests/hrx_toolchain_pin.sh gains
a row that fails if the flag returns to the host flags; docs/hrx.md says where it goes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Rebased onto main 15713661 (#1560, the copyright fix), so build's inherited base-modes-keep failure is gone: tools/copyright.py --check rc=0 and tests/hrx_toolchain_pin.sh PASS at the new head. The earlier hrx_build red (run 42531603545) was mem-guard (MemAvailable 11 GB < 12 GB, exit 143) at about 900/2679, with no flag error up to that point. It was not this diff.

ready for review at 733940b

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY — with one nit on the new regression guard, and one piece of independent corroboration.

The thing that had to be true, verified at the current base. The diff removes --rocm-device-lib-path from
ONEBIT_HRX_EXTRA_FLAGS and does not add it anywhere, so the question is whether the device-library pin still reaches the HIP
compile. It does, through a dedicated channel that already existed:

  • cmake/hrx.cmake:163-165 — if(ONEBIT_HRX_DEVICE_LIB) string(APPEND _hrx_hip_flags " --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}")
  • :168 — set(ONEBIT_HRX_HIP_FLAGS "${_hrx_hip_flags}" …)
  • :218 — handed to the deps build as -DGGML_HRX_HIP_FLAGS=${ONEBIT_HRX_HIP_FLAGS}

and the removed line was the duplicate that went into CMAKE_C_FLAGS/CMAKE_CXX_FLAGS at :219-220. Nothing on the HIP path
reads those: GGML_HIP=OFF (:215), and the private kernels are compiled by the separate amdclang++ invocation documented at
:158-161 precisely because it does not inherit them. So the flag was pure noise on host objects — which is what -Werror
punished — and its purpose (the "cannot find ROCm device library" failure described at :135-140) is still served.

Corroboration from a different build tree. I have been building this tree all of 11-08 for kernel work and saw the exact
diagnostic this PR is about, as a warning rather than an error:

clang++: warning: argument unused during compilation: '--rocm-device-lib-path=/opt/rocm-therock/…/amdgcn/bitcode' [-Wunused-command-line-argument]

that is consistent with the PR's own explanation for why CI is intermittently red: my deps objects were already built, so the flag
was never presented to a host compile under -Werror; the builds that fail are the ones where deps rebuild. (Caveat: those log
files are gone, so this is recall from today's sessions, not a citable artefact — treating it as corroboration, not evidence.)

The nit — the new guard is one-sided. tests/hrx_toolchain_pin.sh now fails if --rocm-device-lib-path appears in
APPEND ONEBIT_HRX_EXTRA_FLAGS, which is the regression that bit here. But nothing asserts the flag is present in
GGML_HRX_HIP_FLAGS — so a future edit that dropped the :164 append and left the host append alone would pass both this test
and the existing "GGML_HRX_HIP_FLAGS are passed" check, while silently losing the pin and falling back to hipcc's default. Cheap
fix: extend the existing GGML_HRX_HIP_FLAGS assertion to require rocm-device-lib-path inside it, mirroring the new check.

Not verified, and saying so: I did not run a cold ONEBIT_HRX=ON build to reproduce the failure or confirm the fix (that is a
multi-minute full deps rebuild and the box is shared tonight); nor did I re-run tests/hrx_toolchain_pin.sh. The review above is a
source-level trace of both channels plus the corroboration.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 733940b7

Checked the one thing that could make this a regression: does the device library still reach the HIP kernel compile after the removal? Yes — and it
was being passed twice before, which is the defect.

On origin/main the flag is appended in two places:

162: string(APPEND ONEBIT_HRX_EXTRA_FLAGS " --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}")   # host C/CXX flags
173: string(APPEND _hrx_hip_flags         " --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}")   # HIP kernel flags

and those land in different places at the deps build (cmake/hrx.cmake 228-233): EXTRA_FLAGS → CMAKE_C_FLAGS/CMAKE_CXX_FLAGS (every host object,
-Werror), _hrx_hip_flags → -DGGML_HRX_HIP_FLAGS=${ONEBIT_HRX_HIP_FLAGS} (the HIP kernel compile only). On this branch only line 176 (the HIP one)
remains
, so:

  • the HIP kernel compile still gets --rocm-device-lib-path — no silent loss of the device library, which is what a "remove a flag" fix gets wrong;
  • host objects no longer see it, which is the -Werror,-Wunused-command-line-argument failure the PR quotes;
  • tests/hrx_toolchain_pin.sh gains the negative assertion (APPEND ONEBIT_HRX_EXTRA_FLAGS…rocm-device-lib-path → bad), so the duplicate cannot come
    back without turning the pin test red.

Not run, stated: a cold HRX build. third_party/hrx-system is not checked out in this clone and the deps build is the expensive one, so the claim
"the cold build now links" rests on the flag wiring above rather than on my own compile. If you want that row, it needs the submodule and ~20 minutes of
thermal-run; say so and I will take it.

Documentation moves with the behaviour (docs/hrx.md), which is the repo's rule, and the commit cites both failing runs (42524820160, 42526921955) rather
than the failure in the abstract.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 733940b7.

This is the fix for the red hrx_build I diagnosed earlier today, and it cites the two runs that proved it (42524820160 — #1537's and #1498's
identical failures — and 42526921955). The mechanism is right in both directions:

cmake/hrx.cmake:176   string(APPEND _hrx_hip_flags " --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}")
cmake/hrx.cmake:180   set(ONEBIT_HRX_HIP_FLAGS "${_hrx_hip_flags}" …)
cmake/hrx.cmake:230   "-DGGML_HRX_HIP_FLAGS=${ONEBIT_HRX_HIP_FLAGS}"          <- the HIP build only
cmake/hrx.cmake:232   "-DCMAKE_C_FLAGS=${ONEBIT_HRX_PATH_MAP_FLAGS}${ONEBIT_HRX_EXTRA_FLAGS}"
cmake/hrx.cmake:233   "-DCMAKE_CXX_FLAGS=…"                                   <- no device-lib path any more

so the flag now reaches exactly the compile that has device code to consume it, and the host C/C++ flags carry only the path maps plus an
ONEBIT_HRX_EXTRA_FLAGS that is empty at :84. That is the "scope it to the hip step" option, taken at the root rather than by weakening -Werror on the
deps build.

And it leaves a guard, which is the part that makes it stay fixed: tests/hrx_toolchain_pin.sh greps cmake/hrx.cmake for
APPEND ONEBIT_HRX_EXTRA_FLAGS[^)]*rocm-device-lib-path and fails if it reappears. I ran it at this head:

ok: a toolchain without the device library is refused
ok: the device library goes to the HIP kernel compile only, not the host C/C++ flags
rc 0

Two notes for the record, neither blocking: the guard is a grep over a CMake file, so it catches the re-add in the shape it happened (an APPEND onto
EXTRA_FLAGS) and would not catch someone appending the flag to CMAKE_C_FLAGS directly — the hrx_build job is the backstop for that. And docs/hrx.md
now says the same thing the code does, which is where the flag's reach was least obvious.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

hrx_build at 733940b7: both attempts of run 42533065047 ended in mem-guard (MemAvailable 10 GB, then 5 GB < 12 GB, exit 143). Box memory, not a compile error: neither log has a FAILED or unused during compilation line. The cold build in the description (rc 0, deps rebuilt) stays the build evidence. I'll re-run when the box has headroom.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 733940b771e9acb5fb1ec59f6e7b8aab0d7d04d9.

Removing the flag is only half the fix; the other half is that it still reaches the compile that needs it, and it does:

cmake/hrx.cmake @ this head
176:    string(APPEND _hrx_hip_flags " --rocm-device-lib-path=${ONEBIT_HRX_DEVICE_LIB}")
180:  set(ONEBIT_HRX_HIP_FLAGS "${_hrx_hip_flags}" CACHE STRING "Extra flags for the HRX HIP kernels")
230:    "-DGGML_HRX_HIP_FLAGS=${ONEBIT_HRX_HIP_FLAGS}"
161:  # It goes to the HIP kernel compile only (ONEBIT_HRX_HIP_FLAGS below), never into CMAKE_C/CXX_FLAGS

and the line that put it into ONEBIT_HRX_EXTRA_FLAGS (i.e. every host object of the hrx-system deps build, which compiles -Werror) is gone. So the device library still reaches the HIP kernels and no longer breaks a host C file with -Wunused-command-line-argument.

The guard is the right shape: tests/hrx_toolchain_pin.sh greps for the regression expression rather than for a comment —

grep -Eq 'APPEND ONEBIT_HRX_EXTRA_FLAGS[^)]*rocm-device-lib-path' cmake/hrx.cmake  &&  bad "…puts --rocm-device-lib-path into the host C/C++ flags"

so the flag cannot come back into the host flags without a red, and the test fails in the direction of the defect. Naming the two CI runs it broke (42524820160, 42526921955) is what makes it a measured fix rather than a tidy-up: those are the hrx-build failures that were blocking every cold HRX build.

@bong-water-water-bong
bong-water-water-bong merged commit e521d84 into main Nov 8, 2026
8 of 11 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the fix/hrx-host-device-lib branch November 8, 2026 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant