Skip to content

Bump HRX: hrx-system 170d229a3 — three libhrx/amdgpu profiling fixes (engine #894, #895, #1232) - #1239

Merged
bong-water-water-bong merged 2 commits into
mainfrom
1bit/bump-hrx-system-170d229a
Nov 6, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
1bit/bump-hrx-system-170d229a

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Moves third_party/hrx-system along our line to a commit with the three measured profiling fixes. third_party/llama.cpp is unchanged.

from to
third_party/hrx-system (1bit-MONSTER/hrx-system 1bit/e3e6a976-pin-6) 82804197bcbe 170d229a3afe

170d229a3 is the pin branch after three fork merges, each measured on strixhalo before it landed:

  • engine#1232 — libhrx ends a live profiling session when the last device handle is released, before hal_device is cleared (fork PR hrx-system#12). A C-API client's bundle goes 770 B / 0 dispatch events → 40 568 B with session-end, command-buffer and dispatch records; the engine's own caller is unaffected because its explicit hrx_device_retain keeps its release out of that branch. No-op when profiling is off.
  • engine#894 — the HIP-binding submission path retains profile metadata and writes its events out (fork PR hrx-system#13, the retargeted HRX: follow AMD's live ggml-hrx, pinned to AMD's tested pair and kept current #11). Stream workload bundle 770 B / 0 events → 948 574 B / 1 230 dispatch events.
  • engine#895 — the dispatch profiling ring is reclaimed on exhaustion (flush + retry) and drops only a submission that cannot fit, instead of killing the session at 65 536 events (fork PR hrx-system#10). Stock: 3×200 then 8× HTTP 500 at 65 508 recorded events; fixed: 12×200 and 106 863 events in one session, same workload and hold.

Same AMD core (e3e6a9766ccb); all three changes sit on profiling paths (sessions/rings/device release), so the inference path is untouched when HRX_PROFILE_MODE is unset.

Verification / merge gate

This is an hrx-system pin move, so build requires config/hrx-long-prompt-validated-pin.config to name the new commit (tools/check_pins.py). That record is deliberately not in this first commit — it is written only after the hardware gate below passes on this branch, so build is red until then and the PR cannot merge before it has been validated on the box. The hw-gate label is added: its phases are build,ops,kat,kld,needle,placement on the strixhalo-engine runner (scripts/hw-gate.sh), i.e. the long-prompt/needle correctness check this repo requires for an hrx-system pin plus test-backend-ops -b HRX0.

Second commit (after the gate): validated-pin config + any doc line. Do not merge on build alone.

Fork PRs

Also updates config/hrx-system-hold.config to name the new commit, so the daily bump does not move off this line.

@bong-water-water-bong bong-water-water-bong added the hw-gate Run the HRX hardware gate on Strix Halo (docs/hw-gate.md) label Nov 6, 2026
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit f5bae21)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

894 - Partially compliant

Compliant requirements:

  • A HIP-path capture now contains dispatch events when profiling is enabled.
  • The binding's stream command buffer now retains profile metadata when a profiling session is active.

Non-compliant requirements:

  • The documentation in docs/hrx.md has not been updated to reflect the limitation and the fix.

Requires further human verification:

  • The documentation update in docs/hrx.md needs to be verified.

895 - Partially compliant

Compliant requirements:

  • The dispatch profiling ring is now reclaimed on exhaustion (flush + retry) and drops only a submission that cannot fit, instead of killing the session at 65 536 events.

Non-compliant requirements:

  • The client-visible error message has not been updated to include the ring and number details.
  • The documentation in docs/hrx.md has not been updated to state the ceiling.

Requires further human verification:

  • The error message update and documentation in docs/hrx.md need to be verified.

1232 - Partially compliant

Compliant requirements:

  • A still-active session is now ended in hrx_device_release's refcount-0 branch, before hal_device is cleared.
  • The C-API client's bundle now contains dispatch events and a session-end record.

Non-compliant requirements:

  • The documentation in docs/hrx.md has not been updated to reflect the new behavior regarding the borrowed-handle contract.

Requires further human verification:

  • The documentation update in docs/hrx.md needs to be verified.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Documentation Clarity

The commit message in config/hrx-system-hold.config provides a detailed explanation of the changes, but it's important to ensure that the documentation is updated in docs/hrx.md to reflect the new behavior regarding profiling limits and session handling, as mentioned in the ticket descriptions.

# #1535 question stays where it was, and the long-prompt check passed at this commit
# (tests/hrx_long_prompt.py PASS twice, 7082-token prompt, needle 7341).
# 2026-10-18, engine#315 candidate 1: the hold names 82804197b, which is 2ab311496 plus
# hrx-system#8 - util/no_svm_gate.c, HRX_ALLOW_NO_SVM=1 lets the amdgpu HAL open a ROCr that
# reports no SVM (HSA_USE_SVM=0), so serve can put the CWSR save area in GTT on a CWSR-on box
# (app/hrx_cwsr.h). Unset, nothing changes. Same pin line, same AMD core.
# The long-prompt check passed at this commit (tests/hrx_long_prompt.py PASS twice, 7082-token
# prompt, needle 7341, 2026-10-18 on strixhalo).
# 2026-11-06, engine#894/#895/#1232: the hold names 170d229a3, which is 82804197b plus three
# libhrx/amdgpu profiling fixes, each measured on this box before it merged (fork PRs #10, #13, #12):
#   - engine#1232: a live profiling session is ended when the last device handle is released, before
#     hal_device is cleared (a C-API client's bundle went 770 B -> 40 568 B with command/dispatch records);
#   - engine#894: the HIP-binding submission path retains profile metadata and writes its events out
#     (bundle 770 B / 0 events -> 948 574 B / 1 230 events on the stream workload);
#   - engine#895: the dispatch profiling ring is reclaimed on exhaustion (flush + retry) and drops only
#     the submission that cannot fit, instead of killing the session at 65 536 events (stock: 8x HTTP 500
#     at 65 508 events; fixed: 12x 200 and 106 863 events in the bundle).
# Same AMD core (e3e6a9766ccb); the profiling paths only, the workload paths are untouched. The bump is
# validated by the hw-gate phases (build,ops,kat,kld,needle,placement) on strixhalo before merge.
170d229a3afec62344ae39c5bfad9bdd16c60f71

@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 dc22a6ef.

The pin topology is what the body says, checked against the fork rather than the prose:
compare/82804197bcbe...170d229a3afe -> 6 commits = the three feature commits (204376342 #895 ring reclaim,
23b26f0c8 #894 retain/write, 2e240c010 #1232 profile-end-on-release) plus their three merge commits
(170d229a3 #12, 549b8be1b #13, f4828c03b #10). So "three fork merges, each measured" is three merges of three
commits, and the branch tip is the merge you pinned.

The two places that must not drift do not: third_party/hrx-system moves 82804197 -> 170d229a, and
config/hrx-system-hold.config's hold names 170d229a3afec62344ae39c5bfad9bdd16c60f71 — the same commit, which is the
pair the hold config exists to keep in step. third_party/llama.cpp is untouched, as the body says, so this cannot drag
the engine's decoder pin with it.

What I did not do is build the HRX path against the new submodule — that is a full ggml-hrx + addon compile, and the
numbers this PR rests on (770 B -> 40 568 B, 948 574 B / 1 230 events, 12x 200 and 106 863 events past the 65 536 ring)
are profiling-bundle measurements that only mean something with a device window and the stream workload. Those are the
author's; the structural half — right commit, right tip, hold and pointer agreeing, decoder pin untouched — is checked.

@bong-water-water-bong
bong-water-water-bong force-pushed the 1bit/bump-hrx-system-170d229a branch from dc22a6e to f5bae21 Compare November 6, 2026 05:25
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

hw-gate PASS at 170d229a3 — run https://github.com/1bit-MONSTER/engine/actions/runs/42164191959

The gate ran on this branch (1bit/bump-hrx-system-170d229a) with the new hrx-system pin, on the strixhalo-engine runner, and finished success at 05:25Z:

phase result
build PASS (7 s)
ops (test-backend-ops -b HRX0) 2083/2083 tests passed (MUL_MAT, MUL_MAT_ID, FLASH_ATTN_EXT, GATED_DELTA_NET, SSM_CONV, GET_ROWS, ROPE, RMS_NORM, SOFT_MAX)
fa-decode-q8 6/6 (K q8_0 × V f16 and q8_0; Hq/Hkv 16/8 and 32/8; n_kv 1031/2048/4103)
kat / needle / placement / kld — qwen3-0.6b PASS (needle 7082-token expected 7341 got 7341; 170 graphs all on HRX; KLD 0.000001)
kat / needle / placement / kld — zaya1-8b PASS (7403-token needle 7341; 193 graphs; KLD -0.000000)
kat / placement / kld — qwen3.8-27b PASS (248 s; 152 graphs; KLD -0.000000)
remaining K-quant and MoE rows PASS (job success)

This is the long-prompt/needle check tools/check_pins.py requires for an hrx-system pin move, so the second commit now records the validated pin and updates the hold file's commit note; check_pins.py against main reports ok third_party/hrx-system: 82804197b -> 170d229a3 is ahead. Ready to merge on build.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit f5bae21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hw-gate Run the HRX hardware gate on Strix Halo (docs/hw-gate.md) Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant