optimizer rework - #25
Closed
Human9000-bit wants to merge 25 commits into
Closed
Conversation
First commit of the IR arc, landing before any of it so every later change is checked against it. Optimizer passes are currently asserted structurally — "the graph now has one fewer node", "op_type is Conv_Relu". That misses the failure that matters: a transform which fuses the right nodes and computes the wrong thing. Only rewrite_convtranspose_dts had a numerical check, and it was written inline. Generalizes that check into tests/equivalence.rs: assert_transform_preserves_numerics loads the model twice, runs the reference before mutating anything, applies the transform, and compares outputs under an explicit Tolerance — Exact for transformations that only reorganize execution and must not touch arithmetic at all, Abs for rewrites that legitimately re-associate it. assert_transform_is_noop covers the other half: a pass presented with a graph outside its safe subset must decline, and the common failure there is firing anyway. Lcg replaces the hand-rolled inline generator; seeded, so failures reproduce. A harness that cannot fail proves nothing, so harness_self_tests exercises each assertion against a deliberately broken transform: swapping Relu for Sigmoid (same node count, different arithmetic) must trip the value check, swapping it for GlobalAveragePool must trip the shape check, and appending a node must trip the no-op check. Four of the five self-tests are #[should_panic]. rewrite_convtranspose_dts_is_numerically_identical and rewrite_convtranspose_dts_skips_unsafe_cases now go through the harness, which also retires the Russian-language comments in those two tests. Validation: cargo fmt, clippy -p yscv-onnx --all-targets --all-features -D warnings, cargo test -p yscv-onnx (186 passed).
Passes mutated OnnxModel directly, whose edges are String tensor names with no index over them. Answering "who consumes this value?" meant rescanning every node's input list, so six passes carried a verbatim copy of the same O(N²) loop, and matching was restricted to positional adjacency (nodes[i], nodes[i+1]) — which is the only reason reorder_nodes_for_fusion has to run first. Adds crates/yscv-onnx/src/ir/: a graph where edges are ValueIds and every value carries its defining node plus its complete use list, so producer/consumer queries are O(1) and matching no longer depends on node order. NodeId and ValueId index append-only vectors and removal tombstones the slot, so ids stay valid across mutations and a pass can collect candidates in one sweep and rewrite them in another without the index-invalidation dance Vec::remove forces. Op interns the 34 operators passes actually branch on; the long tail stays a string in Op::Other, so lowering round-trips any graph the loader accepts. The def-use index is redundant state, so Graph::validate recomputes it by brute force and every mutation checks itself against that behind a debug assertion. ir/pass.rs replaces the hard-coded twelve-call sequence with a Pass trait and a driver that sweeps to a fixpoint, capped at three sweeps so two passes that undo each other cannot hang the loader. YSCV_ONNX_PASSES takes -name entries to disable a pass while bisecting a bad model without a rebuild; YSCV_ONNX_PASS_LOG=1 reports what each sweep did. OnnxModel stays the public type and the runner's input, gaining to_ir/apply_ir. Initializers become Constant values rather than a side table, so passes ask graph.constant(v) instead of looking a name up in a map. apply_ir deliberately leaves the loader's weight-layout side tables and the runtime index alone: layout tags are keyed by weight name and passes preserve those names, and the index is rebuilt once by the driver. Three passes ported, chosen because they exercise the whole API rather than to be exhaustive — the rest follow. eliminate_dead_code drops from O(sweeps · N) with a string hash per edge to an O(N + E) worklist. remove_dropout_nodes and eliminate_squeeze_unsqueeze_pairs can no longer express the two bugs fixed in the previous commit: rewiring is replace_all_uses_with on a value id, and candidates are node ids, so neither name-keyed deletion nor shifted-index removal is representable. eliminate_squeeze_unsqueeze_pairs also gains reach — it now matches a pair separated by an unrelated node, verified by driving the pass alone on an unreordered graph rather than through optimize_onnx_graph, where reorder_nodes_for_fusion would have made the pair adjacent anyway. The two representations coexist for now: the string passes run first, then the model is lowered once and the IR pipeline runs. Each pass ported later moves from one list to the other. A pass failure leaves the model untouched and reports on stderr, since optimize_onnx_graph returns (); that should become a real error return once constant folding — which executes the graph, so it can fail — moves over. Validation: 214 yscv-onnx tests, 2305 workspace. New numerical- equivalence coverage via the harness from the previous commit on the dropout, squeeze/unsqueeze-chain and non-adjacent-pair fixtures, all at Tolerance::Exact. IR tests cover def/use correctness, fan-out and graph-output refusal in sole_consumer, id stability across removal, per-port tracking for repeated operands, omitted optional inputs, and lowering round-trips including unknown operators. Two tests corrupt the index by hand to confirm validate actually rejects. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, clippy with `gpu` and with `gpu rknn native-camera`, check-doc-counts.sh (onnx-cpu-ops still 122 — no dispatch arms changed).
The plan for this stage was to delete fuse_conv_relu and fuse_bn_relu on
the grounds that layer 3 re-derives the activation. It does not.
build_runtime_index computes NodeAction::Conv { activation } purely from
NodeKind::ConvRelu / ConvSilu (runtime_index.rs:896-901), which come from
the op_type string these passes write; the only pattern it matches on its
own is Conv -> Add -> Relu. Deleting them would have dropped Conv+Relu
fusion entirely, costing a full extra output pass per convolution on
every CNN. They are ported instead, and the deletion moves to the stage
that takes ownership of fusion selection.
The two passes had identical bodies — find the producer, confirm the
activation is its only consumer, retag, steal the output — differing only
in which operator absorbs the Relu, and each carried its own copy of the
O(N) consumer rescan. They collapse into one FuseActivation
parameterized twice.
The rewiring needed a new IR primitive. Doing it with set_input/set_output
cannot keep the def-use invariants valid between steps, since two nodes
would transiently claim the same output value; rewiring the other way
(replace_all_uses_with on the activation's output) validates fine but
renames the value, which silently renames a model output when the fused
node sits at one. Graph::absorb_consumer does it atomically: the producer
inherits the consumer's output edges, name included, and the intermediate
value is orphaned. It debug-asserts the caller established sole
consumption, which sole_consumer already checks.
fuse_conv_relu and fuse_bn_relu leave the public API. Nothing outside the
crate called them — quantize-cli and llm-bench go through
optimize_onnx_graph — and AGENTS.md rules out keeping shims.
reorder_enables_fusion_on_interleaved_branches no longer tested its
premise: def-use matching fuses those pairs whether or not anything
reordered them first. Split into interleaved_branches_both_fuse, which
pins the end-to-end result, and reorder_restores_producer_consumer_-
adjacency, which pins the property layer 3 actually still depends on —
build_runtime_index matches nodes[i+1]/nodes[i+2], so reordering stays
load-bearing until that moves onto the IR.
Validation: 216 yscv-onnx tests, 2305 workspace. New coverage: fusion
declines when the intermediate has a second reader; the fused node keeps
the activation's output name when that name is a graph output; and the
interleaved-branch fixture is checked at Tolerance::Exact, since
annotation fusion moves no arithmetic. cargo fmt, clippy --workspace
--all-targets --all-features -D warnings, check-doc-counts.sh
(onnx-cpu-ops still 122 — Conv_Relu and BatchNormalization_Relu are
still produced, so no dispatch arm changed).
Last of the passes that had to run before the string-based ones. On the IR the algorithm shrinks: the producer map and the consumer lists that reorder_nodes_for_fusion built by hand are just value.def and value.uses, so the pass is the depth-first topological sort and nothing else. It reports a change only when the order actually differs, which matters now that it runs under a fixpoint driver — reporting a change unconditionally would burn every sweep. Its purpose has narrowed, and the doc comment now says so. IR passes match through the use list, so they no longer need producer/consumer adjacency; the layer-3 plan builder still does, since build_runtime_index matches nodes[i+1] and nodes[i+2] for FusedDwPw / FusedPwDw / ConvAdd. That is the only reason the pass survives, and it retires when plan construction moves onto the IR. Moving it into the pipeline meant splitting that pipeline in two. The remaining string passes match positionally, so they need the reorder to have run; the activation fusions need to run after the folds. One pipeline cannot satisfy both, so optimize_onnx_graph now runs ir_cleanup_passes (dropout, squeeze/unsqueeze, dead code, reorder), then the string-based folds, then ir_fusion_passes (activation fusion, dead code, reorder). Two lowerings instead of one; both are O(N) and negligible against the ten runtime-index rebuilds this arc removed. The phases collapse back into one as the folds move over. Graph::set_order comes back with it, rejecting anything that is not a permutation of the live nodes — a short order would drop a node from execution and a duplicate would run one twice. Renamed IrError's variants (StaleDef, InconsistentUses, NotAPermutation); clippy's enum_variant_names fired on the shared Bad prefix once the third arrived. Validation: 217 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Folding rewrites a weight in place, and the string version rewrote
initializers[name] without asking who else pointed at that name. Two
Convs sharing one weight — the two branches of a Siamese tracker, the
exact shape reorder_nodes_for_fusion exists to serve — would each fold
their own BatchNormalization into it, applying the second scale on top of
the first. The IR answers "who else uses this?" in O(1), so the pass now
declines when the weight or bias has more than one use. Fewer folds on
weight-sharing models, correct results on them.
Two smaller latent bugs go with it. The synthesized bias name was
format!("{}_fused_bias", conv.name), and ONNX node names are optional, so
every unnamed Conv proposed the same name and the second silently aliased
the first's tensor; Graph::fresh_value_name now derives a name that is
actually free. And the pass no longer needs producer/consumer adjacency,
so a Conv and its BatchNormalization separated by an unrelated node fold
where they previously did not.
The three-way layout branch is gone. Conv weights are pre-permuted by the
loader into one of four layouts, recorded in three side sets on
OnnxModel, and every weight-rewriting pass carried its own copy of the
branch chain to find the output-channel axis — fold_conv_bn,
fold_conv_mul, fold_conv_add_const and graph_cost, four copies, one of
them with different indices. ir::WeightLayout holds it once, with
out_channels and channel_of, tagged onto the value during lowering. The
module is marked for deletion: the permute is a kernel concern that
belongs in plan construction, and once it moves there the passes see
logical OIHW and the tag is unnecessary.
Splitting the pass into match_fold and apply keeps every rejection before
any mutation, so a decline cannot leave a half-rewritten weight.
Validation: 223 yscv-onnx tests, 2305 workspace. fold_conv_bn is checked
at Tolerance::Abs(1e-5) — folding re-associates arithmetic — and the
shared-weight fixture is checked at the same bar with an assertion that
neither BatchNormalization folded. WeightLayout has its own tests: each
layout reads the right axis, and channel_of partitions the buffer evenly
for all four, which is what makes a folded scale land on the right
weights. cargo fmt, clippy --workspace --all-targets --all-features
-D warnings, check-doc-counts.sh.
fold_conv_mul and fold_conv_add_const were separate files sharing a matcher, a layout branch chain and an O(N) consumer rescan, differing only in whether the constant scales the weight or just shifts the bias. They become one FoldConvConstBinary parameterized twice. The port turned up a correctness bug. broadcast_scale_to_oc accepted any constant whose element count equalled the output-channel count, including a bare rank-1 [OC]. ONNX broadcasting aligns trailing axes and a 2-D convolution's output is NCHW, so [OC] aligns against *width*: it scales columns, not channels, and only broadcasts at all when OC happens to equal the output width. Folding it into the weights as a per-channel scale computed something else entirely. The rule is now positional — a scalar, or a constant whose channel-aligned axis holds OC with every other axis 1, so [1, OC, 1, 1] and [OC, 1, 1] qualify and [OC] does not. Inherits the fixes from the BatchNormalization port: declines when the weight or bias is shared with another Conv rather than applying this constant to that one too, names a synthesized bias via Graph::fresh_value_name instead of a node-name format string that collides across unnamed nodes, and matches through the use list so a Conv and its Mul separated by an unrelated node still fold. Only rewrite_convtranspose_dts and fold_constants are still on the string representation. A note on the test shape: the numerical check uses a scalar constant, because the runtime's elementwise kernels require equal shapes, so the *unfolded* reference cannot execute a broadcast constant at all and there is nothing to compare against. The per-channel cases get structural coverage over both operators and both operand orders, plus an explicit test that a rank-1 [OC] constant is refused. Validation: 226 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Answering the question of whether folding by execution is the right approach: yes, and it stays. The alternative is a const-eval interpreter over some whitelist of operators — a second implementation that then has to be kept in lockstep with runner/dispatch.rs, which is the class of duplication this whole migration exists to remove. ORT folds by execution for the same reason. The machinery around each evaluation was the problem. Per foldable node the old pass built a throwaway OnnxModel, called rebuild_runtime_index() on it — the entire plan builder: fusion scan, slot assignment, weight prepacking — then run_onnx_model, which sets up thread pools, output masks and tower-parallel probing. All to run one operator. It now dispatches straight onto a bare TensorEnv via execute_node_kind: no model, no index, no plan. TensorEnv::for_const_eval supplies that, since from_model needs a built index; insert already allocates a dynamic slot for unknown names, so empty model tables are enough. Four further defects went with it. The scan restarted from index 0 every iteration; folding makes a node's output constant, which can only enable *later* nodes, and ReorderForFusion leaves node_ids() topological, so one forward sweep now reaches the same fixed point. `Err(_) => break` aborted all remaining folding on the first operator the runner declined — an unevaluatable node is now simply skipped. There was no cap on the result, so Expand or Tile over a constant could inflate an initializer without bound. And the Random*/Multinomial family was foldable, which would have frozen one draw into the weights. Eligibility stays a denylist. An allowlist would fold strictly less than before and silently regress models relying on the wider reach; the size cap bounds what a misjudged operator can cost. rewrite_convtranspose_dts ports too, which needed Graph::replace_node: one node becoming two, where neither half alone defines the original's output, so the splice has to be atomic and the last replacement inherits the outputs by identity rather than by rename. With those two the string representation is gone from the optimizer. The two IR phases collapse into one pipeline, and the ordering that survives is documented as what saves a sweep rather than what is required — the driver sweeps to a fixed point and no pass depends on adjacency. ReorderForFusion is last because it serves the layer-3 plan builder, which is still positional. remove_node's precondition needed widening, and the IR's own debug assertion caught it: after folding, a value's consumers legitimately remain, because a constant is a definition. The rule is now that each output must be unused, a graph output, or constant. optimize_onnx_graph returns Result<(), OnnxError>. Constant folding can genuinely fail and the old signature had nowhere to say so — errors were printed to stderr and the model came back silently unoptimized. Six call sites updated in the crates that had them, each matching its file's existing error style. Note the distinction: a node the evaluator declines is not foldable and is skipped; only a structurally broken graph is an error. Validation: 229 yscv-onnx tests, 2305 workspace. New coverage: a constant chain collapses in one sweep and is numerically equivalent; an unevaluatable node no longer stops later folds; a constant node feeding a graph output keeps its producer so the name survives lowering. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh (onnx-cpu-ops still 122). README, architecture.md and CHANGELOG updated in this commit.
ONNX attribute names are a closed, per-operator vocabulary — strides, pads, axis, kernel_shape — but they were spelled as string literals at every one of the ~200 sites that read or wrote one. A typo produced a silently missing attribute and a default value rather than a compile error, and there was no single place to see which names the runtime understands. The camelCase outliers made it worse: transA and transB sit next to forty snake_case names, so "transb" reads correctly and behaves as absent. Attr interns all 41 of them and keys the map, so OnnxNode.attributes is FxHashMap<Attr, OnnxAttribute> and the four get_attr_* helpers take an Attr. The name/variant table lives in one macro, so from_name and as_str cannot drift apart, and the string only exists at the two boundaries that need it: proto decode and export. Attr::Other keeps names the runtime does not interpret, so decoding and re-exporting a model with unfamiliar attributes is still lossless — the same shape as Op::Other in the IR. The public field type changes, so this reaches apps/llm-bench's two local attribute helpers, which are retyped the same way. Deliberately not converted: attribute *values* that read as enums — "CRD", "zeros", "bilinear", "avg" — are compared at exactly one site each, so four per-operator enums would be ceremony rather than safety. Pass names stay &'static str: they are identifiers for humans, matched against YSCV_ONNX_PASSES, not a vocabulary the compiler needs to check. Tensor and node names stay String, since those are genuinely open. Validation: 233 yscv-onnx tests, 2305 workspace. Attr has its own tests for exact round-tripping, name uniqueness, Other passthrough, and preservation of the camelCase spellings. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Loading and planning are separate concerns that shared a module. The loader decodes a model; the plan decides how to run it — classifying each node, selecting fusions, resolving convolution parameters, prepacking weights. Keeping both in loader/ is why "the load-time graph optimizer" was a doc comment on a function inside the file that parses protobuf. Behaviour-preserving move. NodeAction, NodeKind, ConvParams, RuntimeModelIndex and the two FusedPwDwPwReduce types go to plan/mod.rs; runtime_index.rs becomes plan/build.rs. loader/mod.rs drops from 691 lines to 375 and is now about parsing a model, which is what its name says. build.rs gains explicit imports rather than inheriting the loader's via `use super::*`, so what it actually depends on is visible. No logic changed and no function was split — this is the module boundary Stage 3 needs before the 1771-line builder is broken into phases, done separately so that diff is reviewable as logic rather than as movement. Validation: 233 yscv-onnx tests, 2305 workspace, unchanged. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Two phases lift cleanly out of the 1779-line plan builder, each depending only on the graph and neither on anything the fusion scan decides. assign_slots interns every tensor name to a dense id — which is what lets the per-inference environment index a Vec instead of hashing strings — pre-resolves each node's inputs and outputs to those ids, classifies operator types once into NodeKind, and records use counts so the runner can free a tensor as soon as its last reader has run. It also assigns the tower-parallel branch tags; that is scheduling rather than indexing, but it depends only on the graph, so splitting it into a third sweep would buy nothing. resolve_conv_params reads strides, pads, group and the depthwise/pointwise classification once, keeping attribute lookups out of the per-inference Conv dispatch. Its doc comment records why it is awkward: it has to interpret the weight's shape against whichever of four layouts the loader pre-permuted it into — the same leak ir::WeightLayout works around, and the one a later phase is meant to remove by moving the permute into prepack_weights. build.rs drops from 1779 to 1550 lines. What remains is the fusion scan, its post-passes and weight prepacking — deliberately left whole, because select_fusions is about to be rewritten onto def-use matching and splitting it first would mean splitting code that is then replaced. No logic changed; the phases are the same statements in the same order, with their outputs named. The bodies moved verbatim apart from resolve_conv_params gaining node_kinds as a parameter, which it previously read from the enclosing scope. Validation: 233 yscv-onnx tests, 2305 workspace, unchanged. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Four std::env::var calls sat inside the per-inference loop. The worst is YSCV_TRACE_SHAPES in execute_node_with_layout_kind, which runs for every node of every inference: each call allocates a String and takes the environ lock purely to discover the variable is unset. On a 200-node model that is 200 allocations per run to answer a question whose answer cannot change. The three on the Conv dispatch path — YSCV_BNNS, YSCV_INDIRECT_MAX_COUT, YSCV_NCHWC_DW — are the same shape; only YSCV_NCHWC_DW is uncfg'd, so it is the one that fires on this target, once per depthwise convolution per inference. All four now go through OnceLock, matching the pattern already used a few lines away for YSCV_RESHAPE_NHWC_PASSTHROUGH_OFF (layout.rs:47). These switches select a compute path, so a mid-run change would alter results underneath the caller anyway; reading them once is more honest about that than pretending they are dynamic. No perf win is claimed, and none could be measured. Three consecutive benchmark runs on the same host — the last two on byte-identical code — drifted monotonically slower on all four models (yolov8n 73.3 -> 74.6 -> 79.7 ms, resnet18 32.6 -> 33.3 -> 34.8 ms), so the machine was thermally decaying faster than the effect being measured. The change is justified structurally instead: a OnceLock read is strictly cheaper than env::var, so the worst case is neutral. It should be re-measured on a cold host before anyone cites a number. This is also the part of Stage 3's `select_kernels` that does not depend on layout. Hoisting the kernel *choice* into the plan does: the dispatch branches on env.is_nhwc() and on input channel count, so it cannot be resolved before assign_layouts and shape inference. The plan lists select_kernels ahead of assign_layouts; that order is wrong and the plan has been corrected. Validation: 233 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Both fusion sites found their partner by looking at the next non-skipped node and then breaking. That is adjacency, not dataflow: it depended on reorder_nodes_for_fusion having placed the pair together, and silently declined whenever any unrelated node was scheduled between them. On a graph with two independent branches — where a topological sort can legitimately interleave them — an entire inverted bottleneck ran unfused. Both sites already required the intermediate value to have exactly one reader, which makes that reader unique, so it can simply be looked up. A sole_consumer index built once alongside the existing producers map turns each search into an O(1) lookup that finds the partner wherever it sits. The index deliberately records only single-reader values: a fan-out value cannot be fused into any one of its readers, and storing the rest would invite a caller to pick the wrong one. plan_fuses_depthwise_pointwise_across_an_unrelated_node pins the behaviour with a `dw, interloper, pw` fixture. Verified against the old matcher, which emits three separate actions (Conv, Generic(Relu), Conv) where this emits FusedDwPw. Measured on a thermally degraded host, so the comparison is hot-vs-hot against the immediately preceding run rather than absolute: mobilenet-v3-small -3.0% (p = 0.00), 2.94 -> 2.85 ms; yolov8n +1.8%, yolo11n +0.2%, resnet-18 +0.5%, none significant. The split is what the mechanism predicts — MobileNetV3 is inverted bottlenecks, which is what these two fusions target, and the other three are not DW/PW-dominated. Worth re-confirming on a cold host before the number is quoted anywhere, since three consecutive runs earlier drifted 4.5-8.8% on identical code. This is the first half of select_fusions moving onto dataflow. The remaining positional matches — ConvAdd on nodes[i+1], the quantized chains — are unchanged here. Validation: 234 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
ConvAdd required the residual Add at nodes[i + 1] and its optional Relu at nodes[i + 2]. A residual block therefore lost its in-place fusion whenever the schedule placed anything between the Conv and its Add, which a topological sort of a branching graph legitimately can — and losing it costs an entire output memory pass, since the fused form writes the sum into the Conv's own output buffer. Both steps already required their intermediate to have exactly one reader, so both are now looked up through the same sole_consumer index the DW+PW and PW+DW sites use. That leaves the quantized chains as the only positional matches in the fusion scan. plan_fuses_conv_add_across_an_unrelated_node pins it, verified against the old form, which emits Conv, Generic(Relu), Generic(Add) where this emits ConvAdd. No perf number, because this host cannot currently produce one. It oscillates roughly 8-10% between thermal states — larger than the effect being measured — and criterion compares against the previous run's saved baseline, so the drift reads as signal in whichever direction the machine happened to move. The run for this change reported -8% on yolov8n and yolo11n, but their absolute times had merely returned to the earlier cold values (74.6 and 58.8 ms against a 73.3 and 57.8 ms cold baseline), so that is the machine recovering, not the change. Measuring anything at this magnitude needs a fixed cooldown between runs, or interleaved A/B. The structural claim stands on the tests, which are deterministic: both fusions now fire on graphs where the positional matcher provably did not. Validation: 235 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
6d9c71c turned the DW+PW partner search from a `for j` scan into a lookup and deleted the trailing `break` that ended the scan. It left behind the *other* break in that loop — the back-off taken when the pointwise half would instead form the stronger `ConvAdd` — which now exits the node loop itself. Plan construction stops dead at the first depthwise conv feeding a pointwise conv feeding an Add, and `build_runtime_index` returns a plan covering only the nodes before it. On a MobileNet-shaped model that is the first inverted bottleneck with a residual, so almost the whole graph goes unplanned; the fixture here comes back with an empty plan for four nodes. The back-off now just declines the fusion, which is what it always meant. While here, it asks its question through the consumer index instead of at nodes[j + 1]: it is predicting what the ConvAdd matcher will do, and that matcher went to dataflow in 4acc5f8, so a positional guess disagrees with it exactly when the Add is not adjacent — the DW+PW fusion would fire and the residual fusion it deferred to would never happen. plan_covers_every_node_when_dw_pw_backs_off_to_conv_add pins both the plan length and a successful run over the fixture. Validation: 236 yscv-onnx tests, 2305 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
The six INT8 chain actions walked their links positionally — the DequantizeLinear at i + 1, then the optional Relu, the QuantizeLinear and the closing QLinearConv at the indices after it, and for the residual suffix a fixed run of i + 1 through i + 5. Any node the schedule placed inside a chain cost the whole fusion, and an INT8 backbone then ran as individual QLinearConv / DQ / Q dispatches: on the fixture here, eleven Generic actions where there should be three. Each link now follows the value it produces to the node that reads it. Nearly all of them already required that value to have exactly one reader before absorbing it, which makes the reader unique, so the walk is a lookup in a consumer index rather than a search. QuantizedForkPair is the exception by construction — the boundary it fuses is one whose value has a side consumer — so it asks for the unique QuantizeLinear among the readers instead. Two matchers were reaching past guards they never stated, both of the same kind: the fused kernel writes only the chain's final output and never materializes the values inside it, so an intermediate with a second reader leaves that reader looking for a tensor nothing produces. QuantizedResidualChain checked dq/relu/conv outputs but not the Add's, and QuantizedForkPair checked none of them before absorbing a Relu. Both now require a single reader and no model-output membership across every value they cross, which is what walking through sole_consumer states directly. quantized_pw_dw_chain_fuses_across_unrelated_nodes runs the existing bitwise PW+DW fixture with an unrelated node scheduled between every pair of links. The dataflow is unchanged, so the chain must still fuse and still match the unfused path bit for bit. Verified against the old matcher, which emits eleven Generic actions and fuses nothing. The fixture is hoisted out of quantized_pw_dw_chain_bitwise_matches_unfused so both tests share it. Validation: 237 yscv-onnx tests, 2306 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
The fusion scan pushes exactly one action per node, which makes plan position and node index the same thing. The transpose post-pass assigns through `execution_plan[node_idx]` on that basis, and the runner walks the plan rather than the node list — so a plan that comes up short does not fail, it silently stops executing the tail of the graph. fa7ce08 fixed a stray `break` that did exactly that. A debug assertion after the scan states the invariant where it is established, so the next early exit is caught by the test suite instead of by a wrong output.
A residual block gives its Add two Conv producers — main path and shortcut — and each is the only reader of its own output, so each one considered alone looks exactly like a ConvAdd. The fused action runs at the Conv's position and reads the other branch out of the environment, so only the later Conv may claim the Add. Fusing the earlier one schedules the Add before the other branch has run, and it reads a tensor nothing has written yet. Adjacency hid this: the Add had to sit at conv_idx + 1, which only the later producer satisfies. 4acc5f8 matched by dataflow and dropped that without replacing it, so resnet-18 stopped loading altogether — "missing input .../shortcut/convolution/Conv_output_0". Both dataflow fusions that absorb a node with a second activation input now check it: ConvAdd for the residual, QuantizedResidualChain for the Add inside the chain. The rest are safe as they stand, since everything else they absorb reads only weights and scales. `available_at` states the rule once — a value is ready at a plan position when its producing node sits earlier, which holds transitively because a fusion is anchored at the first node of its chain and only absorbs nodes after it. plan_fuses_conv_add_with_the_later_producer_of_a_residual pins which of the two Convs owns the Add and checks the arithmetic. Verified against the unguarded matcher, which picks the main-path Conv. Also checked end to end, because a plan-shape bug does not have to fail loudly: yolov8n, yolo11n, resnet-18 and mobilenet-v3-small run to completion and their outputs are identical to main, element sums and all. Validation: 238 yscv-onnx tests, 2307 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
The three streaming fused conv kernels work internally in blocked NCHWc16 on AVX-512. When the next action can read that form directly, the pair skips a round trip through NHWC — a full output pass saved on the hottest part of a MobileNet-shaped backbone. Deciding whether to leave the output blocked was done per action per inference, by walking forward through the plan and pulling weight shapes back out of the tensor environment, in three near-identical copies in plan_branch.rs. None of that walk is dynamic. The plan structure is fixed once the FusedPwDwPwReduce merge has run, and the shapes it reads belong to initializers, so the whole thing is a constant recomputed a few hundred times a second. resolve_nchwc_handoff answers it once at load and stores a bool per plan position; the runner reads the table. This is the first piece of assign_layouts, and it is the shape the rest of that phase takes: layout is a property of a value, decided from the ops that produce and consume it, all of which the plan knows. Indexed by plan position rather than node index on purpose. The two agree through the fusion scan, but the FusedPwDwPwReduce merge drops the actions it absorbs, so by the time this runs they have diverged. Verified by transcribing the old walk and diffing the two predicates over yolov8n, yolo11n, resnet-18 and mobilenet-v3-small: identical, including the single handoff mobilenet actually has. Worth doing that rather than trusting the ports, because the development host has no AVX-512 — the flag is computed here but never consumed, so no local test or benchmark would have caught a mistake. nchwc_handoff_is_resolved_at_plan_time pins the predicate itself, on a fixture that chains two blocked-eligible blocks and one that misses the channel gate. Validation: 239 yscv-onnx tests, 2308 workspace. Model outputs still identical to main. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
The last positional matcher in the plan builder. FusedPwDwPwReduce finds the PW reduce that closes an inverted bottleneck, and it took the first non-skipped node after the depthwise — adjacency, the same pattern the rest of the fusion scan moved off in 6d9c71c and b683634. A node scheduled between the depthwise and its consumer cost the merge, and the block fell back to two actions plus a separate Conv. The merge already requires the DW output to have exactly one reader, so that reader is unique and is a lookup. Declining when it is already claimed by another fusion preserves what the scan did: it walked past skipped nodes and then found nothing reading the DW output. plan_merges_pw_dw_pw_reduce_across_an_unrelated_node puts a Relu on an independent branch between the depthwise and the PW reduce, and checks the merged block against the same graph with YSCV_FUSED_PW_DW_PW_REDUCE_OFF — the merge rewrites how the block is computed, so structure alone is not enough. Verified against the old matcher, which emits FusedPwDw + Generic(Relu) + Conv. No effect on the benchmark models: their plans are byte-identical before and after, because ReorderForFusion already leaves the depthwise adjacent to its consumer on all four. What this buys is that the merge no longer depends on it having done so. Validation: 240 yscv-onnx tests, 2309 workspace. Model outputs still identical to main. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
A single ONNX Conv resolves to one of several yscv-kernels entry points depending on group, kernel size, padding, weight layout and whether a load-time prepack exists. That choice was made inside the dispatch on every inference and was only *observable* afterwards, through a thread-local cell the profiler read back — so nothing could ask which kernel a Conv would use without running it. plan::kernels::resolve_conv_kernels answers it once, per node, from the values the plan already holds. Two conditions looked like they needed the activation shape and do not: with group == 1 the weight's I/G is the input channel count, which settles the first-layer RGB case, and the depthwise test `group == out_channels && group == input_channels` is `in_per_group == 1` for any well-formed Conv. The BNNS path stays a runtime pre-check — it is chosen before the input is converted to NHWC, from whether the input happens to already be NCHW. The dispatch still computes its own answer and note_kernel holds the two against each other on every Conv in debug builds, rather than the plan silently taking over. Replacing those branches with a match on the plan is then mechanical, and this commit is what makes it safe. This is the structural half of select_kernels, and it is worth being plain that it is only that: the per-Conv weight-layout re-derivation it sets up for removal measures 1.7 us per inference on yolov8n, 0.0023% of 73.7 ms. What it buys is a plan that says what it will do before it does it — the thing a cost model would compare, and the thing a new kernel now plugs into by adding a case to one table instead of a branch to a hot function. planned_conv_kernels_match_the_dispatch_on_real_models runs resnet-18 and mobilenet-v3-small under the assertion; both cover padded, prepacked, depthwise and grouped paths. Verified it bites: inverting one branch of the resolver fails with "plan resolved conv kernel Some(NhwcGemm) but dispatch took NhwcPadded". Ignored by default since it needs the downloaded benchmark assets, and fails with a pointer to download-assets.sh when they are missing rather than passing vacuously. Validation: 240 yscv-onnx tests, 2309 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
7d64afa resolved the Conv entry point at load time but left the dispatch computing its own answer alongside, held together by a debug assertion. Two copies of the same conditions is exactly the drift that assertion existed to catch, so this collapses them. plan::kernels::select_conv_kernel is now the one place a Conv entry point's condition lives, taking a ConvShape that both callers can build — the plan from ConvParams plus the initializer shape, the dispatch from the scalars it has already resolved. conv_compute_nhwc takes the plan's answer when there is one and asks the same function when there is not, which is the case for the fused halves and the slow path, neither of which has a plan entry. The aarch64 indirect and NCHWc-depthwise gates become `kernel == ConvKernel::X` instead of restating their own conditions, and every leaf reports the resolved kernel rather than recomputing a label. Adding a Conv kernel is now a variant on ConvKernel, an arm in select_conv_kernel, and the call itself. No predicate is written twice. The kernel API is untouched by design: selection is resolved before the call, so `yscv-kernels` entry points keep taking tensors and scalars and callers do not construct a config to invoke one. planned_conv_kernels_match_the_dispatch_on_real_models still passes, now checking that the plan and the fallback agree rather than the plan and a duplicate. Model outputs identical to main on all four. Validation: 240 yscv-onnx tests, 2309 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
95c2e16 claimed to close the last positional matcher. It missed one: the FusedPwDwPwReduce merge still found its residual Add at pw_reduce_idx + 1 while every other matcher had moved to dataflow, so the two disagreed in both directions. When ConvAdd claimed a non-adjacent Add, the merge absorbed the PW reduce without noticing it. The retain pass that drops actions whose node was absorbed then deleted the whole ConvAdd, and the Add's own plan slot was already Skip. The addition simply never ran: the model returned Ok with a graph output missing and nothing reporting it. The mirror case: an Add adjacent to the PW reduce whose residual operand is produced between the PW expand and the reduce. The merged action runs at the PW expand, so it read a tensor nothing had written and the run failed with MissingInput. ConvAdd declines this through available_at (3ba1a2e); the merge had no such check. Both are regressions from 6d9c71c and 4acc5f8, which moved the partner search to dataflow without the merge following. Neither is reachable on the four benchmark models: ReorderForFusion's DFS happens to leave the Add adjacent, which masks both. That masking is accidental and disappears under YSCV_REORDER_FUSION_OFF=1. The merge now takes over whatever ConvAdd resolved rather than re-deriving it, so the two cannot disagree about which Add belongs to which Conv, and declines outright if it cannot absorb that Add. The availability check is re-run at the *merge's* anchor: ConvAdd validated the operand at the PW reduce's position, and the merged action runs earlier, so that result does not carry over. A debug assertion in the retain states the invariant the drop depends on. Also adds assert_plan_fusion_preserves_numerics, the plan-level counterpart of assert_transform_preserves_numerics. The harness compares output *name sets*, which is what catches a fusion that absorbs a node without computing it — the existing plan tests asserted plan shape and one named output, so the malformed plan was exactly the shape they were asserting. It builds the plan twice under a kill switch, refuses a comparison where the switch changed nothing, and serializes on a shared ENV_LOCK since set_var is not thread-safe and cargo test is concurrent. Verified against the unfixed builder: the non-adjacent case fails with "output names diverged, left: [mid, y] right: [mid]" and the inside-the-block case with MissingInput. Validation: 242 yscv-onnx tests, 2311 workspace. Model outputs identical to main. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
QuantizedForkPair, QuantizedResidualChain, QuantizedConvDq and FusedTransposeMatMul had no test anywhere. Three of the four had their matchers rewritten from positional to dataflow in b683634, so the guards that rewrite added were themselves unverified. Writing the tests turned up two defects. FusedTransposeMatMul fired even when the Transpose had a consumer that could not absorb it. The matcher's own comment says it should not — "AND every consumer of that Transpose is a MatMul that can absorb it" — and treats that as a profitability rule, but it is load-bearing: the fused action reads the value *before* the Transpose, and when some consumer forces the Transpose to run anyway, running it consumes that pre-transpose value if this was its last use. The fused action then reads a tensor that is gone. A Transpose feeding both a MatMul and a Relu fails with MissingInput. This is pre-existing — main produces the same plan and the same error — and is fixed here by checking every reader of the transposed value is an absorbing MatMul before emitting the action. QuantizedForkPair absorbed a Relu without checking the dequantized value it consumes is not a graph output. b683634's message claims otherwise: "Both now require a single reader and no model-output membership across every value they cross." That was true of QuantizedResidualChain and not of this one. Every sibling matcher carries the check; this one now does too. The quantized tests compare the fast path against YSCV_QUANT_INT8_FAST=0 bitwise, and compare which outputs came back — a chain matcher that absorbs a node without materializing what it produced drops a graph output, and comparing only the values of the outputs that did arrive would not notice. Validation: 246 yscv-onnx tests, 2315 workspace. Model outputs identical to main. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Commandment 5 asks for docs in the same change as the code. Several comments were left describing the arrangement they replaced, which is worse than no comment: a reader deciding whether a matcher can rely on node order would have got the wrong answer from all three. optimize_onnx_graph still described a three-phase migration with passes "still string-based, still matching nodes[i + 1]", and linked [ir_cleanup_passes] and [ir_fusion_passes] — neither of which exists, so both were broken intra-doc links. There is one pipeline and no string-based passes. reorder_nodes_for_fusion still claimed the plan builder matches nodes[i + 1] and nodes[i + 2] for FusedDwPw / FusedPwDw / ConvAdd, and that the pass could retire once that changed. It has changed; the pass does not retire, because what the schedule now decides is peak memory — the same nodes in a different topological order leave a different number of live ranges overlapping. The name should follow the purpose once the buffer arena gives something to measure a rename against. plan/build.rs called itself "still one large function", four phases after that stopped being true. Also documents YSCV_ONNX_PASSES, YSCV_ONNX_PASS_LOG, YSCV_REORDER_FUSION_OFF and YSCV_FUSED_PW_DW_PW_REDUCE_OFF in docs/feature-flags.md, which commandment 5 names as the canonical list and which had none of them; and tidies CHANGELOG's Unreleased section, which had grown two separate Changed headings. Validation: 246 yscv-onnx tests, 2315 workspace. cargo doc no longer warns for this crate's optimizer. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
plan_merges_pw_dw_pw_reduce_across_an_unrelated_node failed intermittently in the full suite while passing in isolation. It asserts the shape of a plan it just built, and a sibling test was toggling YSCV_FUSED_PW_DW_PW_REDUCE_OFF — the switch that decides that shape — concurrently. Two mistakes, both mine, both in the commit that added assert_plan_fusion_preserves_numerics. The harness locked only around the window where it set the variable, on the theory that setting is the dangerous part. Reading is equally dangerous here: plan construction consults these switches, so any test that builds a plan and asserts something about it needs the lock too, for its whole body and not just the part that writes. And it introduced a second mutex over a resource that already had one. Two independent locks over one global serialize neither against the other, so the qlinear tests' lock did not exclude the plan tests at all. Both are now the same lock, and it is handed out as an EnvGuard that the harness demands by reference. That makes the requirement type-checked rather than remembered — a caller cannot forget to hold it, and cannot deadlock by taking it twice, because the guard is unforgeable outside the module. Verified by running the suite 12 times before the fix (one failure) and 10 times after (none). Validation: 246 yscv-onnx tests, 2315 workspace. cargo fmt, clippy --workspace --all-targets --all-features -D warnings, check-doc-counts.sh.
Human9000-bit
force-pushed
the
onnx-optimizer-ir
branch
from
August 7, 2026 13:17
c2825d1 to
696f43f
Compare
Owner
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Big optimizer rework!!!!
Before
Right now, all optimizations are being performed on ONNX model. It is quite simple approach, but we have to restrict ourselves due to risk of invalidating the model. Also it is quite hard there to add new fused kernels.
Now
Structure
This is heavily inspired by compilers,
rustcin particularNow there are 3 levels of representations:
rustc's.Some things for follow-up PRs (probably will end up in tracking issue)
tracking issue: #29