From 636150d4acd099299b03e93689b78fd02cf22ca2 Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 04:39:19 -0400 Subject: [PATCH 1/8] hrx: claim a head-row ROPE only when a fused matcher accepts its parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fused_context_claim` accepted a per-head ROPE on SHAPE alone (`is_head_row(op) && fed_by_op(op)`), while the two registrations that execute rotations accept different PARAMETER sets: * llm/dispatch-attention-qkv.cpp: `n_dims == head_size && mode == NORMAL`, ext_factor 0, attn_factor 1; * common/dispatch-rope-set-rows.cpp: `4 <= n_dims <= head_size`, multiple of 4, mode NORMAL / NEOX / collapsible IMROPE. A vision projector's rotation (`n_dims = 32` on a 64-wide head, `mode = GGML_ROPE_TYPE_VISION`) satisfies neither, but it is a head row fed by an op, so it was claimed and then refused when the dispatch was built: E graph_compute: unsupported HRX node 11: ROPE output=f32[64,16,784,1] <- ROPE inputs=[f32[64,16,784,1] <- VIEW] E graph_compute: also: ROPE matcher rejected node: ROPE params or positions shape is unsupported params={n_dims=32, mode=24, …} E clip_image_batch_encode: ggml_backend_sched_graph_compute failed with error -1 i.e. the whole CLIP graph failed instead of that one node running on the CPU, which is why engine #1467's projector cannot run with HRX0 in the graph at all. The condition now lives in one place, `ggml/src/ggml-hrx/rope-claim-params.h`: the two matcher predicates plus their union. Both matchers forward to it (no behaviour change) and the claim requires the union, so the claim cannot drift away from the matchers again - which is exactly how two earlier attempts at this fix broke text decode: each restated the condition and used only the SET_ROWS predicate, which text rotations do not satisfy because they run inside the attention fusion. Verified without a device (the GPU was held by another lane's bench for this whole window): `tests/test-hrx-rope-claim-params.cpp` (ctest `test-hrx-rope-claim-params`) asserts the predicates directly and drives `fused_context_claim` on a graph shaped like the failing node (3-D, VIEW over a produced tensor, so the head-row and fed-by-an-op rules both hold): * text rotation (n_dims == head width, NORMAL) -> claimed (unchanged by this commit) * vision rotation (n_dims 32, mode VISION) -> NOT claimed <- the fix and, with ONLY this claim change reverted and everything else in place, the test aborts with `claim for vision rotation: got 1, wanted 0` - so it fails on the defect and passes with the fix. Still blocking the rest of #1467: with the projector on HRX the next refused node is a pure-alias VIEW (`unsupported HRX node 20: VIEW f32[2048,2,1,1] <- VIEW`), which is claimed by the standalone path rather than by a fused claim. That is a separate change and is not in this commit. Refs #1467 --- .../common/dispatch-rope-set-rows.cpp | 9 +- .../llm/dispatch-attention-qkv.cpp | 7 +- ggml/src/ggml-hrx/fused-context-claim.h | 19 +- ggml/src/ggml-hrx/rope-claim-params.h | 69 +++++++ tests/CMakeLists.txt | 7 + tests/test-hrx-rope-claim-params.cpp | 173 ++++++++++++++++++ 6 files changed, 275 insertions(+), 9 deletions(-) create mode 100644 ggml/src/ggml-hrx/rope-claim-params.h create mode 100644 tests/test-hrx-rope-claim-params.cpp diff --git a/ggml/src/ggml-hrx/dispatch_registration/common/dispatch-rope-set-rows.cpp b/ggml/src/ggml-hrx/dispatch_registration/common/dispatch-rope-set-rows.cpp index 5595f0e90297..7360dafc65ed 100644 --- a/ggml/src/ggml-hrx/dispatch_registration/common/dispatch-rope-set-rows.cpp +++ b/ggml/src/ggml-hrx/dispatch_registration/common/dispatch-rope-set-rows.cpp @@ -6,6 +6,7 @@ #include "dispatch-layout-utils.h" #include "dispatch-rope-utils.h" +#include "rope-claim-params.h" #include "ggml.h" #include "qwen4exp-hrx-support.h" #include "graph/graph-matcher.h" @@ -129,12 +130,10 @@ static bool is_supported_rope_input_layout(const Value & value, size_t & span_el return required <= span_elements; } +// The condition itself lives in ggml/src/ggml-hrx/rope-claim-params.h, so the claim in +// fused-context-claim.h cannot drift away from what this matcher accepts (engine #1467). static bool is_supported_rope_params(const RopeParams & params, int64_t head_size) { - const bool supported_mode = params.mode == GGML_ROPE_TYPE_NORMAL || params.mode == GGML_ROPE_TYPE_NEOX || - rope_mode_is_collapsible_imrope(params); - return params.n_dims >= 4 && params.n_dims <= head_size && params.n_dims % 4 == 0 && supported_mode && - std::isfinite(params.freq_base) && params.freq_base > 0.0f && std::isfinite(params.freq_scale) && - params.freq_scale > 0.0f && std::isfinite(params.ext_factor) && std::isfinite(params.attn_factor); + return rope_params_supported_by_rope_set_rows(params, head_size); } static bool build_rope_theta_table(const GraphNode & rope, diff --git a/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-attention-qkv.cpp b/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-attention-qkv.cpp index ea81bda320f3..03462b793aed 100644 --- a/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-attention-qkv.cpp +++ b/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-attention-qkv.cpp @@ -4,6 +4,7 @@ #include "ggml.h" #include "graph/graph-matcher.h" #include "kernel-corpus/kernel-corpus-catalog-verify.h" +#include "rope-claim-params.h" #include #include @@ -104,10 +105,10 @@ static std::string to_config_value(int64_t value) { return std::to_string(value); } +// The condition itself lives in ggml/src/ggml-hrx/rope-claim-params.h, because the claim in +// fused-context-claim.h has to assert exactly what this matcher accepts (engine #1467). static bool is_supported_attention_rope_params(const RopeParams & params, int64_t head_size) { - return params.n_dims == head_size && params.mode == GGML_ROPE_TYPE_NORMAL && std::isfinite(params.freq_base) && - params.freq_base > 0.0f && std::isfinite(params.freq_scale) && params.freq_scale > 0.0f && - params.ext_factor == 0.0f && params.attn_factor == 1.0f; + return rope_params_supported_by_attention_fusion(params, head_size); } static bool build_rope_theta_table(const GraphNode & rope, int64_t head_size, std::vector & data) { diff --git a/ggml/src/ggml-hrx/fused-context-claim.h b/ggml/src/ggml-hrx/fused-context-claim.h index f098887a6089..9ab4f23a810a 100644 --- a/ggml/src/ggml-hrx/fused-context-claim.h +++ b/ggml/src/ggml-hrx/fused-context-claim.h @@ -39,6 +39,7 @@ // standalone test. #include "ggml.h" +#include "rope-claim-params.h" #include "ubatch-token-limits.h" #include @@ -155,8 +156,24 @@ inline bool fused_context_claim(const ggml_tensor * op) { op->ne[2] <= 3 && op->ne[0] >= 32 && op->ne[0] <= 65536 && op->ne[0] % 32 == 0; // per-head attention normalisation and rotation case GGML_OP_RMS_NORM: - case GGML_OP_ROPE: return fused_context_is_head_row(op) && fused_context_fed_by_op(op); + case GGML_OP_ROPE: { + // A head-row rotation is claimed only when some fused registration accepts its PARAMETERS as well as + // its shape: a claim that is broader than the matchers leaves the node with no dispatch, and the + // whole graph fails instead of the node falling back to the CPU. engine #1467 is that failure: the + // Qwen3-VL projector's ROPE (n_dims 32 against a 64-wide head, mode GGML_ROPE_TYPE_VISION) is a + // head row fed by an op, so it was claimed, and both matchers then rejected it - "ROPE matcher + // rejected node: ROPE params or positions shape is unsupported" - which made clip_image_batch_encode + // fail on HRX0 at chunk 25 instead of running that node on the CPU. The predicates are shared with the + // matchers (rope-claim-params.h) rather than restated here, because restating them is what broke text + // decode twice: text rotations satisfy the attention fusion's set, not the ROPE+SET_ROWS one. + if (!fused_context_is_head_row(op) || !fused_context_fed_by_op(op)) { + return false; + } + const OpParams imported = import_op_params(*op); + const RopeParams * params = op_params_as(imported); + return params != nullptr && rope_params_supported_by_any_fusion(*params, op->ne[0]); + } default: return false; } diff --git a/ggml/src/ggml-hrx/rope-claim-params.h b/ggml/src/ggml-hrx/rope-claim-params.h new file mode 100644 index 000000000000..b2517de047bb --- /dev/null +++ b/ggml/src/ggml-hrx/rope-claim-params.h @@ -0,0 +1,69 @@ +// Copyright 2026 bong-water-water-bong +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +// Which ROPE parameter sets can a fused registration actually execute? +// +// The claim in `fused-context-claim.h` says "this node will run inside a fused dispatch", and +// `device_supports_op` uses that to keep the node on this backend. If the claim is broader than the +// matchers, the node is claimed and then refused when the dispatch is built, and the whole graph +// fails instead of the node falling back to the CPU - engine #1467: a vision projector's ROPE +// (`n_dims = 32`, `mode = GGML_ROPE_TYPE_VISION`) was claimed by the head-row rule and then rejected +// by both matchers, so `clip_image_batch_encode` could not compute at all on HRX0. +// +// Two matchers accept rotations and their parameter sets differ: +// +// * the attention fusion (llm/dispatch-attention-qkv.cpp): n_dims == head_size, mode NORMAL; +// * the ROPE + SET_ROWS registration (common/dispatch-rope-set-rows.cpp): 4 <= n_dims <= head_size, +// n_dims a multiple of 4, mode NORMAL / NEOX / a collapsible IMROPE. +// +// Their union is what the claim may assert. Both predicates live here, the two matchers call them and +// the claim calls the union, so a matcher can be narrowed without the claim silently going stale - +// which is how the two earlier attempts at this fix broke text decode: they re-derived the condition +// and used only the SET_ROWS one, which text rotations do not satisfy (they run inside the attention +// fusion). + +#include "graph/op-params.h" +#include "qwen4exp-hrx-support.h" // rope_mode_is_collapsible_imrope + +#include + +namespace ggml::hrx { + +// llm/dispatch-attention-qkv.cpp: the per-head rotation consumed by the attention fusions. +inline bool rope_params_supported_by_attention_fusion(const RopeParams & params, int64_t head_size) { + return params.n_dims == head_size && params.mode == GGML_ROPE_TYPE_NORMAL && std::isfinite(params.freq_base) && + params.freq_base > 0.0f && std::isfinite(params.freq_scale) && params.freq_scale > 0.0f && + params.ext_factor == 0.0f && params.attn_factor == 1.0f; +} + +// common/dispatch-rope-set-rows.cpp: the rotation that registration builds a theta table for. +inline bool rope_params_supported_by_rope_set_rows(const RopeParams & params, int64_t head_size) { + const bool supported_mode = params.mode == GGML_ROPE_TYPE_NORMAL || params.mode == GGML_ROPE_TYPE_NEOX || + rope_mode_is_collapsible_imrope(params); + return params.n_dims >= 4 && params.n_dims <= head_size && params.n_dims % 4 == 0 && supported_mode && + std::isfinite(params.freq_base) && params.freq_base > 0.0f && std::isfinite(params.freq_scale) && + params.freq_scale > 0.0f && std::isfinite(params.ext_factor) && std::isfinite(params.attn_factor); +} + +// What the claim may assert: at least one fused registration accepts these parameters. Anything else +// (a vision rotation, a mode nobody implements) must fall back to the CPU rather than be claimed. +inline bool rope_params_supported_by_any_fusion(const RopeParams & params, int64_t head_size) { + return rope_params_supported_by_attention_fusion(params, head_size) || + rope_params_supported_by_rope_set_rows(params, head_size); +} + +} // namespace ggml::hrx diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c592e6b41034..972e161370de 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -350,6 +350,13 @@ add_test(NAME test-server-health COMMAND test-server-health) if (TARGET ggml-hrx) # the program cache's byte budget and LRU order: pure policy, no device and no HRX link + # Which rotations the fused registrations accept, i.e. what the claim may assert (engine #1467). + # Header-only: no device, no model, and the vision rotation must be rejected here. + add_executable(test-hrx-rope-claim-params test-hrx-rope-claim-params.cpp) + target_include_directories(test-hrx-rope-claim-params PRIVATE ../ggml/include ../ggml/src ../ggml/src/ggml-hrx) + add_test(NAME test-hrx-rope-claim-params COMMAND test-hrx-rope-claim-params) + target_link_libraries(test-hrx-rope-claim-params PRIVATE ggml) + add_executable(test-hrx-graph-program-budget test-hrx-graph-program-budget.cpp ../ggml/src/ggml-hrx/runtime/graph-program-budget.cpp) target_include_directories(test-hrx-graph-program-budget PRIVATE ../ggml/include ../ggml/src ../ggml/src/ggml-hrx) diff --git a/tests/test-hrx-rope-claim-params.cpp b/tests/test-hrx-rope-claim-params.cpp new file mode 100644 index 000000000000..0471d2225bbd --- /dev/null +++ b/tests/test-hrx-rope-claim-params.cpp @@ -0,0 +1,173 @@ +// Copyright 2026 bong-water-water-bong +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Host test for rope-claim-params.h: which rotations the fused registrations accept, and therefore what +// the claim in fused-context-claim.h may assert. No device and no model - the predicates are pure. +// +// The case this exists for is engine #1467: a vision projector's ROPE carries mode +// GGML_ROPE_TYPE_VISION and n_dims 32 against a 64-wide head, which BOTH matchers reject. The claim used +// to accept it on shape alone, so the node was claimed and then refused when the dispatch was built, and +// the whole CLIP graph failed instead of that node running on the CPU. + +#include "fused-context-claim.h" +#include "rope-claim-params.h" + +#include "ggml.h" + +#include +#include + +#define REQUIRE(condition) \ + do { \ + if (!(condition)) { \ + std::fprintf(stderr, "%s:%d: requirement failed: %s\n", __FILE__, __LINE__, #condition); \ + std::abort(); \ + } \ + } while (0) + +namespace { + +using ggml::hrx::RopeParams; + +RopeParams text_rotation(int n_dims, int head_size) { + RopeParams params; + params.n_dims = n_dims; + params.mode = GGML_ROPE_TYPE_NORMAL; + params.freq_base = 1000000.0f; + params.freq_scale = 1.0f; + params.ext_factor = 0.0f; + params.attn_factor = 1.0f; + (void) head_size; + return params; +} + +// The projector rotation: two-dimensional vision rotary, per ggml.h "GGML_ROPE_TYPE_VISION n_dims = 4 --> +// [yyyyxxxx]". The node #1467 fails on is n_dims 32 against head_size 64 with this mode. +RopeParams vision_rotation() { + RopeParams params; + params.n_dims = 32; + params.mode = GGML_ROPE_TYPE_VISION; + params.freq_base = 10000.0f; + params.freq_scale = 1.0f; + params.ext_factor = 0.0f; + params.attn_factor = 1.0f; + return params; +} + +// The claim itself, on a graph shaped like the one engine #1467 fails on: a ROPE whose source is a VIEW +// (so the head-row and fed-by-an-op rules both hold) over a 64-wide head. Pre-fix the claim accepted the +// vision rotation on shape alone and the node was then refused at dispatch build, which failed the whole +// CLIP graph instead of running that node on the CPU. +static void check_claim_for(bool vision, bool want_claimed) { + ggml_init_params params = { /*mem_size*/ 24 * 1024 * 1024, /*mem_buffer*/ nullptr, /*no_alloc*/ false }; + ggml_context * ctx = ggml_init(params); + REQUIRE(ctx != nullptr); + + // 3-D, as the claim's fused_context_is_3d requires - the real node in #1467 is [64, 16, 784, 1], i.e. ne[3] == 1. + ggml_tensor * base = ggml_new_tensor_3d(ctx, GGML_TYPE_F32, 64, 16, 784); + // fed-by-an-op is resolved through the view chain to its ROOT (fused_context_producer_op), so the view has + // to sit on top of a tensor an op produced - a view of a leaf is GGML_OP_NONE and would make the claim false + // for the wrong reason, which is what an earlier version of this test did. + ggml_tensor * ones = ggml_new_tensor_3d(ctx, GGML_TYPE_F32, 64, 16, 784); + ggml_tensor * mul = ggml_mul(ctx, base, ones); + ggml_tensor * view = ggml_view_3d(ctx, mul, 64, 16, 784, mul->nb[1], mul->nb[2], 0); + ggml_tensor * pos = ggml_new_tensor_1d(ctx, GGML_TYPE_I32, vision ? 3136 : 784); + ggml_tensor * rope = ggml_rope_ext(ctx, view, pos, nullptr, vision ? 32 : 64, + vision ? GGML_ROPE_TYPE_VISION : GGML_ROPE_TYPE_NORMAL, 0, 1000000.0f, 1.0f, + 0.0f, 1.0f, 0.0f, 0.0f); + + // the backend the claim asks about: "is the producer supported" - here, yes for everything, so the + // only thing under test is the ROPE rule itself. + const auto supported = [](const ggml_tensor * t) { return t != nullptr; }; + const bool claimed = ggml::hrx::fused_context_claim(rope, supported); + if (claimed != want_claimed) { + std::fprintf(stderr, "claim for %s rotation: got %d, wanted %d (engine #1467)\n", + vision ? "vision" : "text", (int) claimed, (int) want_claimed); + std::abort(); + } + ggml_free(ctx); +} + +} // namespace + +int main() { + using namespace ggml::hrx; + + { + // A text attention rotation: n_dims equals the head width, mode NORMAL. The attention fusion takes + // it, and it is the case that broke twice when the claim was narrowed with the SET_ROWS predicate + // alone (text rotations do not satisfy that one). + const RopeParams params = text_rotation(64, 64); + REQUIRE(rope_params_supported_by_attention_fusion(params, 64)); + REQUIRE(rope_params_supported_by_any_fusion(params, 64)); + } + { + // A head-width NEOX rotation with fewer dims than the head: the attention fusion declines (mode and + // n_dims), the ROPE+SET_ROWS registration takes it, so the claim may still assert it. + RopeParams params = text_rotation(32, 64); + params.mode = GGML_ROPE_TYPE_NEOX; + REQUIRE(!rope_params_supported_by_attention_fusion(params, 64)); + REQUIRE(rope_params_supported_by_rope_set_rows(params, 64)); + REQUIRE(rope_params_supported_by_any_fusion(params, 64)); + } + { + // THE REGRESSION: the vision rotation. Neither matcher accepts it, so the claim must not either - + // this is the assertion that would have caught engine #1467 before an image request crashed. + const RopeParams params = vision_rotation(); + REQUIRE(!rope_params_supported_by_attention_fusion(params, 64)); + REQUIRE(!rope_params_supported_by_rope_set_rows(params, 64)); + REQUIRE(!rope_params_supported_by_any_fusion(params, 64)); + } + { + // The rejection is on the MODE as well as on n_dims: a vision rotation at head width is still not a + // rotation any fused registration executes. + RopeParams params = vision_rotation(); + params.n_dims = 64; + REQUIRE(!rope_params_supported_by_attention_fusion(params, 64)); + REQUIRE(!rope_params_supported_by_rope_set_rows(params, 64)); + REQUIRE(!rope_params_supported_by_any_fusion(params, 64)); + } + { + // n_dims wider than the head is nobody's case. + RopeParams params = text_rotation(128, 64); + REQUIRE(!rope_params_supported_by_any_fusion(params, 64)); + } + { + // A degenerate freq_base is declined by both, so it cannot be claimed. + RopeParams params = text_rotation(64, 64); + params.freq_base = 0.0f; + REQUIRE(!rope_params_supported_by_any_fusion(params, 64)); + params.freq_base = 1000000.0f; + REQUIRE(rope_params_supported_by_any_fusion(params, 64)); + } + { + // ext_factor is only the attention fusion's condition: the SET_ROWS path builds its own theta table + // and takes it. This is the case where the two matchers genuinely differ, so the union is what the + // claim has to use. + RopeParams params = text_rotation(32, 64); + params.mode = GGML_ROPE_TYPE_NEOX; + params.ext_factor = 1.0f; + REQUIRE(!rope_params_supported_by_attention_fusion(params, 64)); + REQUIRE(rope_params_supported_by_rope_set_rows(params, 64)); + REQUIRE(rope_params_supported_by_any_fusion(params, 64)); + } + + // The regression proper: the claim must refuse the vision rotation and keep the text one. + check_claim_for(/*vision*/ true, /*want_claimed*/ false); + check_claim_for(/*vision*/ false, /*want_claimed*/ true); + + std::printf("test-hrx-rope-claim-params: all requirements met\n"); + return 0; +} From 5cd2181a3344e5a55702d23a6ce8d7e268aafb7a Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 06:11:13 -0400 Subject: [PATCH 2/8] hrx: say which relation a refused layout-alias node has, and pin alias elision in a host test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit engine #1467's projector is refused at a node the log names only by shape: E graph_compute: unsupported HRX node 20: VIEW output=f32[2048,2,1,1] <- VIEW inputs=[f32[2048,2,1,1]] consumers=[21:ADD] A layout-alias node is refused there for one of two completely different reasons and the shape cannot tell them apart: it IS an alias (one input, same storage as its input) whose input was not available when the node was visited, or it is not aliasing its input at all and therefore needs a dispatch that no registration provides. The message now says which: ... consumers=[…] alias=same_storage inputs=1 ... consumers=[…] alias=not_aliasing_input inputs=2 `alias=` is `is_layout_alias_node(graph, node)` — the helper the elision path itself calls twelve lines below, not a re-derivation of it. That is deliberate (review point on this PR): a diagnostic that restates the scheduler's rule will eventually describe a scheduler that no longer exists, which is the same argument as sharing rope-claim-params.h between the claim and its matchers in #257. The wording says `not_aliasing_input` rather than "not an alias" because the op here IS a layout-alias op; what is false is the storage relation. Alongside it, `tests/test-hrx-view-alias-elision.cpp` (ctest `test-hrx-view-alias-elision`, links ggml-hrx) builds the graph by hand and pins four storage relations, host-only, no backend: no-op alias, external input : plans <- what can_execute_standalone_op_as_graph builds no-op alias, produced input : plans <- the real graph offset alias, produced input: plans strided view, produced input: REFUSED <- and the refusal lands on the CONSUMER, not the view The first two lines are the useful ones: a lone node's input provenance does NOT decide elision, which refutes the reading that the projector's refusal comes from value_is_available() returning true for a producer-less value (dispatch-scheduler.cpp:102-106). That reading cost this lane a wrong comment on the issue, so it is now an assertion. The last line records that a strided view is elided as well — same_storage() compares the storage ROOT, not the layout — and that the plan is then refused at the consumer; if a consumer ever accepts a strided alias, that assertion is where it becomes visible. Verified: ggml-hrx links clean with -Werror and the test runs green (its status line is the new field through the scheduler's own plan().status.errors()). The device run this diagnostic exists for is on the projector; it printed `alias=not_aliasing_input` for node 20, which is what moved the investigation from "which of two causes" to "ValueMap gave the view a storage of its own". Refs #1467 --- .../ggml-hrx/dispatch/dispatch-scheduler.cpp | 14 ++ tests/CMakeLists.txt | 7 + tests/test-hrx-view-alias-elision.cpp | 142 ++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 tests/test-hrx-view-alias-elision.cpp diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index a35c773cf260..354369ba2bd6 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -96,6 +96,20 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c append_node_summary(stream, graph, consumers[i]); } stream << "]"; + // A layout-alias node is refused here for one of two completely different reasons, and the shape cannot tell them + // apart: it is an alias (one input, same storage) whose input was not available yet, or it is NOT an alias at all + // (a strided view of the same shape) and therefore needs a dispatch that no registration provides, because no + // registration names a layout-alias op as its root. engine #1467's projector is refused at + // VIEW output=f32[2048,2,1,1] <- f32[2048,2,1,1] + // and that line alone did not say which case it was, so the next reader had to reproduce it. This costs one line + // in the message and answers it in the log. + if (is_layout_alias_op(node.op)) { + // is_layout_alias_node() is this predicate exactly (graph/graph.cpp): a message that restates the scheduler's rule + // is a message that will describe a scheduler that no longer exists. Same reasoning as rope-claim-params.h sharing + // its predicates with the matchers in #257. + stream << " alias=" << (is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input") + << " inputs=" << node.inputs.size(); + } return stream.str(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 972e161370de..1d27cfa1e2af 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -399,6 +399,13 @@ if (TARGET ggml-hrx) target_include_directories(test-hrx-loom-jit PRIVATE ../ggml/include ../ggml/src ../ggml/src/ggml-hrx) add_test(NAME test-hrx-loom-jit COMMAND test-hrx-loom-jit) + # How a layout-alias node is judged when its input is PRODUCED vs EXTERNAL: engine #1467's projector is + # refused at a no-op VIEW that the standalone oracle calls supported because it feeds the view from a leaf. + add_executable(test-hrx-view-alias-elision test-hrx-view-alias-elision.cpp) + target_link_libraries(test-hrx-view-alias-elision PRIVATE ggml-hrx ggml hrx::hrx loomc::loomc) + target_include_directories(test-hrx-view-alias-elision PRIVATE ../ggml/include ../ggml/src ../ggml/src/ggml-hrx) + add_test(NAME test-hrx-view-alias-elision COMMAND test-hrx-view-alias-elision) + add_executable(test-hrx-ops test-hrx-ops.cpp) target_link_libraries(test-hrx-ops PRIVATE ggml-hrx ggml hrx::hrx loomc::loomc) target_include_directories(test-hrx-ops PRIVATE ../ggml/include ../ggml/src ../ggml/src/ggml-hrx) diff --git a/tests/test-hrx-view-alias-elision.cpp b/tests/test-hrx-view-alias-elision.cpp new file mode 100644 index 000000000000..8c1946c8a6d1 --- /dev/null +++ b/tests/test-hrx-view-alias-elision.cpp @@ -0,0 +1,142 @@ +// Copyright 2026 bong-water-water-bong +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Host test for how a layout-alias node is treated when its input is PRODUCED vs EXTERNAL, which is the +// divergence engine #1467's projector hits: with the projector on HRX the graph is refused at +// unsupported HRX node 20: VIEW output=f32[2048,2,1,1] <- VIEW inputs=[f32[2048,2,1,1]] consumers=[21:ADD] +// while `can_execute_standalone_op_as_graph` reports the same node supported, because the single-node graph it +// builds feeds the view from an External leaf and `value_is_available()` returns true when a value has no +// producer (dispatch-scheduler.cpp:102-106). +// +// No device: the scheduler, the registry and the graph are all host-side; only the tensors are real ggml +// objects, and no backend is initialised. + +#include "dispatch/dispatch-scheduler.h" +#include "graph/graph.h" +#include "graph/op-params.h" + +#include "ggml.h" + +#include +#include +#include + +#define REQUIRE(condition) \ + do { \ + if (!(condition)) { \ + std::fprintf(stderr, "%s:%d: requirement failed: %s\n", __FILE__, __LINE__, #condition); \ + std::abort(); \ + } \ + } while (0) + +namespace { + +using namespace ggml::hrx; + +const DispatchTarget kTarget{ "gfx1151" }; + +// A graph shaped like the failing one: an external leaf, one op that produces a value on this backend, and a +// no-op layout alias on top of it. `produced` selects whether the alias's input is that produced value (the +// real graph) or a second external leaf (what can_execute_standalone_op_as_graph builds). +enum class ViewKind { NoOpAlias, OffsetAlias, StridedView }; + +bool plans_with_view(ViewKind kind, bool produced) { + ggml_init_params params = { /*mem_size*/ 32 * 1024 * 1024, /*mem_buffer*/ nullptr, /*no_alloc*/ false }; + ggml_context * ctx = ggml_init(params); + REQUIRE(ctx != nullptr); + + // 3 rows so an offset view stays in bounds + ggml_tensor * a = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 2048, 3); + ggml_tensor * b = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 2048, 3); + ggml_tensor * add = ggml_add(ctx, a, b); + ggml_tensor * base = produced ? add : a; + // The three relations that matter: a true alias (same storage), an alias at an offset (same buffers, different + // start - the question is whether ValueMap gives it the same storage), and a genuinely strided view (same shape, + // different nb[1]) which is NOT an alias and therefore needs a real dispatch that no registration provides. + ggml_tensor * view = nullptr; + switch (kind) { + case ViewKind::NoOpAlias: view = ggml_view_2d(ctx, base, 2048, 2, base->nb[1], 0); break; + case ViewKind::OffsetAlias: view = ggml_view_2d(ctx, base, 2048, 2, base->nb[1], base->nb[1]); break; + case ViewKind::StridedView: view = ggml_view_2d(ctx, base, 2048, 2, 2 * base->nb[1], 0); break; + } + REQUIRE(view != nullptr); + ggml_tensor * out = ggml_scale(ctx, view, 0.5f); + + Graph graph; + std::vector no_inputs; + // the leaves + const ValueId va = graph.values().get_or_add_tensor_value(a, ValueKind::External); + const ValueId vb = graph.values().get_or_add_tensor_value(b, ValueKind::External); + // the producer of the alias's input + ValueId vadd = va; + if (produced) { + const ValueId vadd_out = graph.values().get_or_add_tensor_value(add, ValueKind::Transient); + GraphNode & n_add = graph.add_node(GGML_OP_ADD, vadd_out, { va, vb }); + n_add.params = import_op_params(*add); + vadd = vadd_out; + } + // the alias itself + const ValueId vview = graph.values().get_or_add_tensor_value(view, ValueKind::Transient); + GraphNode & n_view = graph.add_node(GGML_OP_VIEW, vview, { vadd }); + n_view.params = import_op_params(*view); + // something that consumes the alias, so the alias is not the graph's output + const ValueId vout = graph.values().get_or_add_tensor_value(out, ValueKind::Transient); + GraphNode & n_scale = graph.add_node(GGML_OP_SCALE, vout, { vview }); + n_scale.params = import_op_params(*out); + (void) no_inputs; + REQUIRE(graph.build_index().success()); + + DispatchScheduler sched; + const bool planned = sched.schedule_graph(graph, kTarget); + if (!planned) { + // print whatever the scheduler recorded, so the diagnostic added for engine #1467 is visible here + for (const std::string & e : sched.plan().status.errors()) { + std::printf(" status: %s" "\n", e.c_str()); + } + } + ggml_free(ctx); + return planned; +} + +} // namespace + +int main() { + // Measured behaviour of the four relations, pinned so a change in the alias rule shows up here and not in an + // image request two days later. + const bool noop_external = plans_with_view(ViewKind::NoOpAlias, false); + const bool noop_produced = plans_with_view(ViewKind::NoOpAlias, true); + const bool offset = plans_with_view(ViewKind::OffsetAlias, true); + const bool strided = plans_with_view(ViewKind::StridedView, true); + + std::printf("no-op alias, external input : %s\n", noop_external ? "plans" : "REFUSED"); + std::printf("no-op alias, produced input : %s\n", noop_produced ? "plans" : "REFUSED"); + std::printf("offset alias, produced input: %s\n", offset ? "plans" : "REFUSED"); + std::printf("strided view, produced input: %s\n", strided ? "plans" : "REFUSED"); + + // A no-op layout alias plans whether its input is an External leaf (the single-node graph that + // can_execute_standalone_op_as_graph builds) or a value this backend produces (the real graph). That REFUTES + // the reading that engine #1467's VIEW refusal comes from value_is_available() returning true for a + // producer-less value: if that shortcut decided it, the produced case would be refused. + REQUIRE(noop_external); + REQUIRE(noop_produced); + // An offset alias is still one storage root, so it is elided too. + REQUIRE(offset); + // A STRIDED view of the same shape is elided as well - same_storage() compares the storage root, not the layout - + // and the refusal then lands on the CONSUMER (see the status line: "unsupported HRX node 2: SCALE"). Pinned as + // the safe outcome it is today: the plan is refused, not silently computed through a wrong layout. If a consumer + // ever accepts a strided alias, THIS assertion is where it should become visible. + REQUIRE(!strided); + return 0; +} From 610c61a716ee8e9d85a8de45e11a937724738710 Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 13:11:10 -0400 Subject: [PATCH 3/8] hrx: say WHICH alias lookup missed, not just that one did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `alias=not_aliasing_input` (previous commit) says a layout-alias node's storage is not its input's. On engine #1467's text tower that is where the investigation stops, because two very different causes print the same word and they need different repairs: * `view_src_unregistered` — the `ggml_tensor` the node's `view_src` points at is not a value in this graph, so `find_alias_source`'s pointer lookup cannot hit and the storage-root fallback has nothing to match. Repair: register the value, or resolve the alias by storage *range* rather than by tensor-pointer identity. * `view_src_registered_but_storage_differs` — it WAS registered and the map still gave the view a storage of its own. Repair: that is a bug in the assignment itself, not in what the graph contains. Both now print distinctly: ... alias=not_aliasing_input(view_src_unregistered) inputs=1 ... alias=not_aliasing_input(view_src_registered_but_storage_differs) inputs=1 `find_tensor()` is public and `Value::tensor` is public (`graph/value-map.h:100-101`, `:84`), so this asks the same map the assignment asked, rather than re-deriving anything. Why it matters: the difference decides whether the next change is "register the missing value" or "match by storage range". Guessing wrong costs a device run each time, and this lane has already spent several. Coverage, stated plainly: the new branch is NOT exercised by `test-hrx-view-alias-elision` — every case there fails *before* the message is built (the strided case is refused at its consumer), and `unsupported_node_message` is file-local so the test cannot call it. What the test does show is that the change does not alter the four relations it pins. Exercising the new branch needs the projector on a device. Stacked on the previous commit (`1bit/1467-view-alias-diag`); merge that first. Refs #1467 --- .../ggml-hrx/dispatch/dispatch-scheduler.cpp | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index 354369ba2bd6..6bc435724f03 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -107,8 +108,23 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c // is_layout_alias_node() is this predicate exactly (graph/graph.cpp): a message that restates the scheduler's rule // is a message that will describe a scheduler that no longer exists. Same reasoning as rope-claim-params.h sharing // its predicates with the matchers in #257. - stream << " alias=" << (is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input") - << " inputs=" << node.inputs.size(); + const char * relation = is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input"; + // WHICH lookup missed, not just that one did. A view can fail to alias for two very different reasons and they + // lead to different repairs: the tensor its view_src points at was never registered as a value in this graph + // (register it, or resolve by storage range), or it WAS registered and the map still gave the view a storage of + // its own (that would be a bug in the assignment itself). engine #1467's text tower hits one of these and the + // field above could not say which. + const char * why = ""; + const Value * out = graph.values().find(node.output); + if (out != nullptr && out->tensor != nullptr) { + const ggml_tensor * src = out->tensor->view_src; + if (relation != nullptr && std::strcmp(relation, "not_aliasing_input") == 0) { + why = (src != nullptr && graph.values().find_tensor(src) != nullptr) + ? "(view_src_registered_but_storage_differs)" + : "(view_src_unregistered)"; + } + } + stream << " alias=" << relation << why << " inputs=" << node.inputs.size(); } return stream.str(); } From 121e069fe4c28f7a51f9f1a6f7acaa9610c57fec Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 14:40:25 -0400 Subject: [PATCH 4/8] hrx: print the two storages and the view's layout for a refused alias node Six hypotheses about engine #1467's refused `VIEW` have died under their own tests (input provenance, offsets, strides, an unregistered view_src resolved by storage range, producer coverage, and the claim). Each one was tested by building the case it predicted and watching it not reproduce. That is the signal to stop predicting and print the two storages. For a layout-alias node whose storage is not its input's, the message now also carries: storages=out7/in3 nb=[4,8192,16384,32768] offs=0 * `storages=outN/inM` - the two `ValueStorageId`s. Equal ids with `alias=not_aliasing_input` would be a contradiction (the map agreed and the message says it did not); different ids are the fact every hypothesis has been guessing at; * `nb=[...]` and `offs=` - what layout the view describes. A view whose strides differ from its input's same-shape tensor is a strided alias, which is a different thing from "the map gave it a storage". With the `alias=` and `why=` fields from the two commits below, one run now answers: which storage the map gave the view, whether it is its input's, whether the view's own `view_src` was registered, and what layout it describes. Coverage, stated plainly: like the previous commit, this field is not exercised by `test-hrx-view-alias-elision` - the host cases fail before a message is built, and `unsupported_node_message` is file-local. What the test does show is that the four pinned relations and the added case still behave. Stacked on `1bit/1467-alias-reason` (#262), itself stacked on #258 and #257. Merge in that order. Refs #1467 --- .../ggml-hrx/dispatch/dispatch-scheduler.cpp | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index 6bc435724f03..7f1495aaf5b7 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -124,7 +125,26 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c : "(view_src_unregistered)"; } } - stream << " alias=" << relation << why << " inputs=" << node.inputs.size(); + // THE FACTS, not another story. Six hypotheses have died on this node, so the message now carries what they were + // each guessing at: the two ValueStorageIds (equal ids mean the map agreed; different ids mean it did not), and the + // output tensor's strides and offset. With `alias=` and the `why=` above, one run says which storage the map gave the + // view, whether it is its input's, and what layout the view describes (engine #1467). + std::string facts; + if (relation != nullptr && std::strcmp(relation, "not_aliasing_input") == 0 && node.inputs.size() == 1) { + const Value * out_v = graph.values().find(node.output); + const Value * in_v = graph.values().find(node.inputs[0]); + if (out_v != nullptr && in_v != nullptr) { + char buf[192]; + const ggml_tensor * t = out_v->tensor; + std::snprintf(buf, sizeof buf, " storages=out%d/in%d nb=[%" PRId64 ",%" PRId64 ",%" PRId64 ",%" PRId64 + "] offs=%zu", + out_v->storage.value, in_v->storage.value, + t != nullptr ? t->nb[0] : 0, t != nullptr ? t->nb[1] : 0, t != nullptr ? t->nb[2] : 0, + t != nullptr ? t->nb[3] : 0, t != nullptr ? t->view_offs : size_t(0)); + facts = buf; + } + } + stream << " alias=" << relation << why << facts << " inputs=" << node.inputs.size(); } return stream.str(); } From 45cd7c1d5a64c3d2498b0bb75993fdc0bb617cc1 Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 15:20:37 -0400 Subject: [PATCH 5/8] tests: reproduce engine #1467's refused VIEW on the host, with the run's own numbers The projector run printed the facts this case encodes: VIEW output=f32[2048,2,1,1] nb=[4,32768,65536,65536] offs=8192 alias=not_aliasing_input(view_src_unregistered) storages=out24/in11 Rows of 2048 f32 taken from a buffer whose rows are 8192 f32 apart (`nb[1]` = 32768 bytes = 4x the width), starting one row in (`offs` = 8192 bytes = 2048 f32), and the wide source is never registered as a value in the graph - which is why `find_alias_source`'s two tensor-identity lookups both miss and the view is given a storage of its own. `ViewKind::WideWindow` builds exactly that shape, so `test-hrx-view-alias-elision` now reproduces the refusal on a machine with no device: no-op alias, external input : plans no-op alias, produced input : plans offset alias, produced input: plans strided view, produced input: REFUSED <- refused at its CONSUMER wide window view (node 20) : REFUSED <- refused AT THE VIEW, as the projector is The assertion is `REQUIRE(!wide)`: that is today's behaviour and it is the defect. The fix is a densify dispatch for a layout-alias node that cannot be elided (copy the strided window into a contiguous transient - `dispatch-copy.cpp`'s append_f32_densify_dispatch already does this for permuted activations), and when it lands this line flips to `REQUIRE(wide)` - which makes the change visible in the test rather than inferred from a log line. Two traps for whoever writes it, recorded here because both produce a WRONG NUMBER rather than a failure: the copy must derive its source offsets from nb[]/offs (a densify that reuses the pre-densify plane offsets copies the wrong rows), and the transient must hold ne[0]*ne[1] contiguous elements of the same type. Refs #1467 --- tests/test-hrx-view-alias-elision.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/test-hrx-view-alias-elision.cpp b/tests/test-hrx-view-alias-elision.cpp index 8c1946c8a6d1..eeada6bd0a97 100644 --- a/tests/test-hrx-view-alias-elision.cpp +++ b/tests/test-hrx-view-alias-elision.cpp @@ -50,7 +50,7 @@ const DispatchTarget kTarget{ "gfx1151" }; // A graph shaped like the failing one: an external leaf, one op that produces a value on this backend, and a // no-op layout alias on top of it. `produced` selects whether the alias's input is that produced value (the // real graph) or a second external leaf (what can_execute_standalone_op_as_graph builds). -enum class ViewKind { NoOpAlias, OffsetAlias, StridedView }; +enum class ViewKind { NoOpAlias, OffsetAlias, StridedView, WideWindow }; bool plans_with_view(ViewKind kind, bool produced) { ggml_init_params params = { /*mem_size*/ 32 * 1024 * 1024, /*mem_buffer*/ nullptr, /*no_alloc*/ false }; @@ -70,6 +70,17 @@ bool plans_with_view(ViewKind kind, bool produced) { case ViewKind::NoOpAlias: view = ggml_view_2d(ctx, base, 2048, 2, base->nb[1], 0); break; case ViewKind::OffsetAlias: view = ggml_view_2d(ctx, base, 2048, 2, base->nb[1], base->nb[1]); break; case ViewKind::StridedView: view = ggml_view_2d(ctx, base, 2048, 2, 2 * base->nb[1], 0); break; + case ViewKind::WideWindow: { + // THE SHAPE engine #1467 actually refuses, with the numbers the run printed: + // VIEW f32[2048,2,1,1] nb=[4,32768,65536,65536] offs=8192 + // i.e. rows of 2048 f32 taken from a buffer whose rows are 8192 f32 apart (nb[1] = 32768 bytes = 4x the + // width), starting one row in (offs = 8192 bytes = 2048 f32). The wide source is never registered as a + // value - the run also printed view_src_unregistered - so both of find_alias_source's tensor-identity + // lookups miss and the view gets a storage of its own. + ggml_tensor * wide = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 8192, 4); + view = ggml_view_2d(ctx, wide, 2048, 2, 32768, 8192); + break; + } } REQUIRE(view != nullptr); ggml_tensor * out = ggml_scale(ctx, view, 0.5f); @@ -124,6 +135,8 @@ int main() { std::printf("no-op alias, produced input : %s\n", noop_produced ? "plans" : "REFUSED"); std::printf("offset alias, produced input: %s\n", offset ? "plans" : "REFUSED"); std::printf("strided view, produced input: %s\n", strided ? "plans" : "REFUSED"); + const bool wide = plans_with_view(ViewKind::WideWindow, true); + std::printf("wide window view (node 20) : %s\n", wide ? "plans" : "REFUSED"); // A no-op layout alias plans whether its input is an External leaf (the single-node graph that // can_execute_standalone_op_as_graph builds) or a value this backend produces (the real graph). That REFUTES @@ -138,5 +151,9 @@ int main() { // the safe outcome it is today: the plan is refused, not silently computed through a wrong layout. If a consumer // ever accepts a strided alias, THIS assertion is where it should become visible. REQUIRE(!strided); + // Documented CURRENT behaviour, and it is the defect: the projector's wide-window view is refused because it aliases + // bytes the graph never registered, and nothing can dispatch a layout-alias op. The densify dispatch that fixes this + // flips this assertion to REQUIRE(wide) - when that lands, this line is where the change is visible. + REQUIRE(!wide); return 0; } From 5431c0f13a035e8a337b1938d4c5411b66f1f857 Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 19:20:40 -0400 Subject: [PATCH 6/8] hrx: drop the merge markers the previous commit carried, keeping this branch's superset block The pin has this message code as #258 landed it; this branch has it with the why=/facts additions, so the superset is the resolution and nothing from the pin is lost. The previous merge commit carried the markers themselves, which is why the tree did not compile - caught by rebuilding before pushing, not by review. --- ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index 226ae7b16c94..7f1495aaf5b7 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -109,7 +109,6 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c // is_layout_alias_node() is this predicate exactly (graph/graph.cpp): a message that restates the scheduler's rule // is a message that will describe a scheduler that no longer exists. Same reasoning as rope-claim-params.h sharing // its predicates with the matchers in #257. -<<<<<<< HEAD const char * relation = is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input"; // WHICH lookup missed, not just that one did. A view can fail to alias for two very different reasons and they // lead to different repairs: the tensor its view_src points at was never registered as a value in this graph @@ -146,10 +145,6 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c } } stream << " alias=" << relation << why << facts << " inputs=" << node.inputs.size(); -======= - stream << " alias=" << (is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input") - << " inputs=" << node.inputs.size(); ->>>>>>> e39659538abf14b3433c0bf32756f7dcffbbc6d7 } return stream.str(); } From 01509e8daf5cf41829c199ff03f0660e5b07c4e3 Mon Sep 17 00:00:00 2001 From: bcloud Date: Mon, 9 Nov 2026 19:52:05 -0400 Subject: [PATCH 7/8] hrx: take the pin's AliasMiss block back, and re-apply only the facts on top of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous merge resolved this hunk to *this branch's* older version, which silently removed what the base added deliberately: the `AliasMiss` switch, i.e. the miss cause read from **assignment-time state**. That is the half an earlier review called blocking, and my resolution brought the forbidden re-ask of the map back with it. The reviewer caught it; this commit fixes it. Resolution now, per file: - `dispatch-scheduler.cpp`, `value-map.{h,cpp}`, `graph-diagnostics.cpp`: **the pin's versions** (AliasMiss + NoAliasSource/ExactViewSrc/RangeFallback, recorded when the storage is chosen); - and on top of the pin's block, only this branch's own addition: `storages=outN/inM nb=[…] offs=…`, which is the fact the six dead hypotheses needed. `why=` still comes from the pin's `AliasMiss`, never from a re-ask. - ``/`` added for PRId64/snprintf, which the pin's file did not need. Rebuilt before committing: `ggml-hrx` links clean with -Werror, and the host test still pins the four relations plus `wide window view (node 20) : REFUSED`. Refs #1467 --- .../ggml-hrx/dispatch/dispatch-scheduler.cpp | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp index 7f1495aaf5b7..022fec706366 100644 --- a/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp +++ b/ggml/src/ggml-hrx/dispatch/dispatch-scheduler.cpp @@ -1,3 +1,5 @@ +#include +#include #include "dispatch-scheduler.h" #include "ggml.h" @@ -6,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -109,38 +110,36 @@ static std::string unsupported_node_message(const Graph & graph, size_t index, c // is_layout_alias_node() is this predicate exactly (graph/graph.cpp): a message that restates the scheduler's rule // is a message that will describe a scheduler that no longer exists. Same reasoning as rope-claim-params.h sharing // its predicates with the matchers in #257. - const char * relation = is_layout_alias_node(graph, node) ? "same_storage" : "not_aliasing_input"; - // WHICH lookup missed, not just that one did. A view can fail to alias for two very different reasons and they - // lead to different repairs: the tensor its view_src points at was never registered as a value in this graph - // (register it, or resolve by storage range), or it WAS registered and the map still gave the view a storage of - // its own (that would be a bug in the assignment itself). engine #1467's text tower hits one of these and the - // field above could not say which. - const char * why = ""; - const Value * out = graph.values().find(node.output); - if (out != nullptr && out->tensor != nullptr) { - const ggml_tensor * src = out->tensor->view_src; - if (relation != nullptr && std::strcmp(relation, "not_aliasing_input") == 0) { - why = (src != nullptr && graph.values().find_tensor(src) != nullptr) - ? "(view_src_registered_but_storage_differs)" - : "(view_src_unregistered)"; + // `aliasing` is the decision; the string is derived from it, never the other way round (review of #262: the + // first version compared the string it had just built to decide whether to look further). + const bool aliasing = is_layout_alias_node(graph, node); + const char * relation = aliasing ? "same_storage" : "not_aliasing_input"; + // WHICH lookup missed, read from the VALUE, not re-asked of the map. The map has grown since the storage was + // assigned, so `find_tensor(view_src)` here can succeed where it failed then and this message would name the + // wrong repair. AliasMiss is recorded at assignment time for exactly this (value-map.h), and it is the half of + // this PR that the review called blocking. + const char * why = ""; + const Value * out = graph.values().find(node.output); + if (!aliasing && out != nullptr) { + switch (out->alias_miss) { + case AliasMiss::NoAliasSource: why = "(no_alias_source)"; break; + case AliasMiss::ExactViewSrc: why = "(aliased_view_src_not_the_input)"; break; + case AliasMiss::RangeFallback: why = "(aliased_storage_root_fallback_not_the_input)"; break; + case AliasMiss::None: break; } } - // THE FACTS, not another story. Six hypotheses have died on this node, so the message now carries what they were - // each guessing at: the two ValueStorageIds (equal ids mean the map agreed; different ids mean it did not), and the - // output tensor's strides and offset. With `alias=` and the `why=` above, one run says which storage the map gave the - // view, whether it is its input's, and what layout the view describes (engine #1467). + // THE FACTS, on top of the recorded reason: six hypotheses died on this node, so the message also carries the two + // ValueStorageIds and what layout the view describes (engine #1467). `why` above is the assignment-time reason + // (AliasMiss), which the pin's version computes and which must not be re-asked of the map here. std::string facts; - if (relation != nullptr && std::strcmp(relation, "not_aliasing_input") == 0 && node.inputs.size() == 1) { - const Value * out_v = graph.values().find(node.output); - const Value * in_v = graph.values().find(node.inputs[0]); - if (out_v != nullptr && in_v != nullptr) { + if (!aliasing && out != nullptr && out->tensor != nullptr && node.inputs.size() == 1) { + const Value * in_v = graph.values().find(node.inputs[0]); + if (in_v != nullptr) { char buf[192]; - const ggml_tensor * t = out_v->tensor; - std::snprintf(buf, sizeof buf, " storages=out%d/in%d nb=[%" PRId64 ",%" PRId64 ",%" PRId64 ",%" PRId64 - "] offs=%zu", - out_v->storage.value, in_v->storage.value, - t != nullptr ? t->nb[0] : 0, t != nullptr ? t->nb[1] : 0, t != nullptr ? t->nb[2] : 0, - t != nullptr ? t->nb[3] : 0, t != nullptr ? t->view_offs : size_t(0)); + const ggml_tensor * t = out->tensor; + std::snprintf(buf, sizeof buf, " storages=out%d/in%d nb=[%" PRId64 ",%" PRId64 ",%" PRId64 ",%" PRId64 "] offs=%zu", + out->storage.value, in_v->storage.value, t->nb[0], t->nb[1], t->nb[2], t->nb[3], + t->view_offs); facts = buf; } } From a1b74a0cf3f26bca9bf13bae05c5a30900714220 Mon Sep 17 00:00:00 2001 From: bcloud Date: Tue, 10 Nov 2026 22:00:53 -0400 Subject: [PATCH 8/8] hrx: let the fused GATED_DELTA_NET take a strided q/k norm (engine#1925) The prefill matcher required the q/k/v dim-2 stride to be exactly packed (`nb[2] == hidden*4`), so a q/k norm fed by a view whose token step is wider than `nb[1]*ne[1]` fell through and the node was left uncovered while its consumer was claimed by the fused dispatch: E graph_compute: unsupported HRX node 160: L2_NORM output=f32[128,16,512,1] hip.l2_norm_f32: declined (consumer is GATED_DELTA_NET (left to the fused matcher)) x ne=[128,16,512,1] nb=[4,512,40960,20971520] off=0 <- nb[2] = 40960 against 8192, 5x The token stride is now bounded below and required to stay float-aligned instead of being required to be packed. That is not a new rule: the DECODE matcher in this same file already states it for v -- v->nb[2] < static_cast(width * head_count) * sizeof(float) || v->nb[2] % sizeof(float) != 0 -- so the prefill guard is brought into line with the proof the file already relies on, rather than inventing one. Nothing else relaxes: `nb[0]`, `nb[1]`, the shared storage, the three storage offsets and `raw_k->nb == raw_q->nb` still hold exactly as before, which is what keeps the relaxation to "the token dim may be strided" and not "the layout may differ". Verified: `libggml-hrx.so` builds clean with -Werror. NOT verified by me, and stated in the PR: the placement census (engine#1925 asks for 2 nodes off HRX / 2 splits) and the pin's 12.14 tok/s decode / 371.6 prefill at 18,782 tokens row -- a matcher relaxation that completes but computes wrongly passes the census and shows up only there, so the device row is the reviewer's gate, not something I am claiming. Refs: engine#1925, engine#679 --- .../llm/dispatch-gated-delta-net.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-gated-delta-net.cpp b/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-gated-delta-net.cpp index d70fee16e761..70cd17e7551a 100644 --- a/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-gated-delta-net.cpp +++ b/ggml/src/ggml-hrx/dispatch_registration/llm/dispatch-gated-delta-net.cpp @@ -215,9 +215,15 @@ static GatedDeltaNetMatch match_gated_delta_net(const Graph & graph, const Graph raw_k->storage_offset != static_cast(width * q_head_count) * sizeof(float) || v->storage_offset != static_cast(2 * width * q_head_count) * sizeof(float) || raw_q->nb[0] != sizeof(float) || raw_q->nb[1] != static_cast(width) * sizeof(float) || - raw_q->nb[2] != static_cast(hidden_size) * sizeof(float) || raw_k->nb != raw_q->nb || + // engine#1925: the dim-2 step may be STRIDED -- a q/k norm fed by a view whose token step is wider than + // nb[1]*ne[1] (measured: nb[2]=40960 against 8192, 5x) -- so the token stride is bounded below and required + // to stay float-aligned instead of being required to be packed. This is the proof the decode matcher below + // already uses for v (see the `v->nb[2] < ... || v->nb[2] % sizeof(float)` term): nothing else in this guard + // relaxes, so nb[0]/nb[1], the shared storage, the three offsets and raw_k->nb == raw_q->nb all still hold. + raw_q->nb[2] < static_cast(hidden_size) * sizeof(float) || raw_q->nb[2] % sizeof(float) != 0 || + raw_k->nb != raw_q->nb || v->nb[0] != sizeof(float) || v->nb[1] != static_cast(width) * sizeof(float) || - v->nb[2] != static_cast(hidden_size) * sizeof(float)) { + v->nb[2] < static_cast(hidden_size) * sizeof(float) || v->nb[2] % sizeof(float) != 0) { return {}; }