Skip to content

fix: keep hrx-system at 98d05d94 (our loom-jit needs its AMDGPU runtime-globals API) - #359

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/hrx-system-pin-98d05d94
Oct 8, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/hrx-system-pin-98d05d94

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Current main does not compile with HRX. #353/#357 moved third_party/hrx-system to 4ba76c18eafe (our fork's 1bit/main, which merged AMD's c0b135a778cc). That merge carries AMD's loom commit 32d0c76a "[Loom] Make AMDGPU runtime globals compiler-owned", which deletes loomc_amdgpu_runtime_global_flags_t and loomc_amdgpu_emit_options_t from loom/binding/c/include/loomc/target/amdgpu/emit.h.

Our ggml/src/ggml-hrx/loom-jit.cpp still uses them, so the pinned pair fails:

loom-jit.cpp:70:5:  error: unknown type name 'loomc_amdgpu_runtime_global_flags_t'
loom-jit.cpp:557:59: error: use of undeclared identifier 'LOOMC_AMDGPU_RUNTIME_GLOBAL_FEEDBACK_CONFIG'
loom-jit.cpp:994:5:  error: unknown type name 'loomc_amdgpu_emit_options_t'
10 errors generated.

GitHub-hosted CI builds the pin without HRX, so it cannot catch this (see bump-hrx.yml); a from-scratch HRX build on strixhalo is what surfaced it.

Fix: restore third_party/hrx-system to 98d05d94a9f9e405683f8aeafc037f5ace0abe4a (our fork's previous revision, which exports the API at loom/binding/c/include/loomc/target/amdgpu/emit.h:37). Verified: recompiling loom-jit.cpp against 98d05d94 succeeds (rc=0). This is the constraint already recorded in OWNER-ACTIONS.md §"engine#336": "leaves hrx-system at 98d05d94 — AMD's hrx-system does not export loomc_amdgpu_runtime_global_flags_t, which our loom-jit path needs."

Pin rollback on purpose. llama.cpp is unchanged at e44c9d01a4d5.

Follow-up (forward direction): port loom-jit.cpp to the compiler-owned API and move hrx-system forward again — tracked separately so the release pin can build now.

#353/#357 moved third_party/hrx-system to 4ba76c18eafe (our fork's 1bit/main,
which merged AMD's c0b135a778cc). That merge includes AMD's loom commit
32d0c76a "[Loom] Make AMDGPU runtime globals compiler-owned", which removes
loomc_amdgpu_runtime_global_flags_t / loomc_amdgpu_emit_options_t.

Our ggml/src/ggml-hrx/loom-jit.cpp still uses those symbols, so the pinned
pair does not compile:

  loom-jit.cpp:70:5: error: unknown type name 'loomc_amdgpu_runtime_global_flags_t'
  loom-jit.cpp:994:5: error: unknown type name 'loomc_amdgpu_emit_options_t'
  10 errors generated.

GitHub CI builds the pin without HRX, so it cannot catch this. 98d05d94 is
our fork's previous revision and exports the API (loom/binding/c/include/
loomc/target/amdgpu/emit.h). Compiling loom-jit.cpp against it succeeds.

This restores the constraint already recorded in OWNER-ACTIONS.md: hrx-system
stays at 98d05d94 until our loom-jit path is ported to the compiler-owned API.
@bong-water-water-bong bong-water-water-bong added the pin rollback Moves a submodule pin backwards on purpose (skips the pin check) label Oct 8, 2026
@bong-water-water-bong
bong-water-water-bong enabled auto-merge (squash) October 8, 2026 05:43
@bong-water-water-bong
bong-water-water-bong merged commit 6ebe320 into main Oct 8, 2026
8 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the fix/hrx-system-pin-98d05d94 branch October 8, 2026 05:46
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

353 - Partially compliant

Compliant requirements:

  • Move third_party/llama.cpp to the correct commit
  • Move third_party/hrx-system to the correct commit
  • Resolve conflicts as described
  • No changes to .github/workflows
  • CI builds without HRX

Non-compliant requirements:

  • Validation on Strix Halo with test-backend-ops -b HRX0 not explicitly shown in diff but required by ticket

Requires further human verification:

  • Validation on Strix Halo with test-backend-ops -b HRX0 needs to be confirmed by human

357 - Partially compliant

Compliant requirements:

  • Both submodules moved to AMD-pinned pair
  • Our own commits applied
  • Conflicts resolved as described
  • No changes to .github/workflows
  • CI builds without HRX

Non-compliant requirements:

  • Validation on Strix Halo with test-backend-ops -b HRX0 not explicitly shown in diff but required by ticket

Requires further human verification:

  • Validation on Strix Halo with test-backend-ops -b HRX0 needs to be confirmed by human

336 - Partially compliant

Compliant requirements:

  • Pin third_party/llama.cpp to correct commit
  • Pin third_party/hrx-system to correct commit to retain needed API
  • Verified with test-backend-ops -b HRX0
  • Verified recompilation of loom-jit.cpp succeeds

Non-compliant requirements:

  • Validation on Strix Halo with test-backend-ops -b HRX0 not explicitly shown in diff but required by ticket

Requires further human verification:

  • Validation on Strix Halo with test-backend-ops -b HRX0 needs to be confirmed by human
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Compatibility Issue

The PR reverts third_party/hrx-system to commit 98d05d94 to maintain compatibility with loom-jit.cpp, which depends on loomc_amdgpu_runtime_global_flags_t and loomc_amdgpu_emit_options_t that were removed in the newer commit 4ba76c18eafe. This is a necessary compatibility fix to prevent compilation errors when building with HRX enabled.

Subproject commit 98d05d94a9f9e405683f8aeafc037f5ace0abe4a

bong-water-water-bong added a commit that referenced this pull request Oct 9, 2026
#361 (c4d9a9b) is titled "Bump HRX: llama.cpp 02f2880c9f42 + hrx-system
4ba76c18eafe (AMD's tested pair)" but its diff changes only
third_party/hrx-system. The llama.cpp gitlink stayed at e44c9d01a4d5, whose
ggml/src/ggml-hrx/loom-jit.cpp still uses loomc_amdgpu_runtime_global_flags_t,
loomc_amdgpu_emit_options_t and LOOMC_AMDGPU_RUNTIME_GLOBAL_*, which
4ba76c18eafe removed. The pinned pair therefore does not compile:

  loom-jit.cpp:70:5: error: unknown type name 'loomc_amdgpu_runtime_global_flags_t'
  loom-jit.cpp:553:1: error: unknown type name 'loomc_amdgpu_runtime_global_flags_t'
  ... 10 errors, all in loom-jit.cpp

GitHub CI builds the pin without HRX (ONEBIT_HRX defaults OFF), so it cannot
catch this class of break; a from-scratch HRX build on strixhalo is what
surfaced it.

Restore third_party/hrx-system to 98d05d94a9f9, the revision #359 pinned and
which exports the API our loom-jit path needs. llama.cpp is unchanged.

Co-authored-by: bong-water-water-bong <bong-water-water-bong@users.noreply.github.com>
bong-water-water-bong added a commit that referenced this pull request Oct 9, 2026
…e new revision dropped (#365)

AMD removes public LoomC symbols without notice: loom 32d0c76a (in hrx-system
4ba76c18) dropped loomc_amdgpu_runtime_global_flags_t and
loomc_amdgpu_emit_options_t, which our ggml/src/ggml-hrx/loom-jit.cpp uses.
CI here builds without HRX, so #353, #357 and #361 merged a submodule pair that
does not compile with ONEBIT_HRX=ON, and #359 and #363 pinned hrx-system back by
hand (KNOWN-ISSUES.md).

The bump now checks, at the header level, that every loomc_* / LOOMC_*
identifier used under ggml/src/ggml-hrx at the new llama.cpp revision is still
declared under loom/binding/c/include at the new hrx-system revision. If one is
missing, the PR moves llama.cpp only, keeps hrx-system where it is, and says so
in its body and in a workflow warning. When neither submodule moves, no PR is
opened (#361 moved one gitlink only).

Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pin rollback Moves a submodule pin backwards on purpose (skips the pin check) Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant