Add: hbg in-graph early-dispatch prerequisites — recorded verdicts and a sorted CSR - #2144
Merged
zhusy54 merged 1 commit intoSep 8, 2026
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change records early-dispatch verdicts in graph definitions, validates those flags, and sorts eligible fanin rows. Scheduler wake scanning now resumes backward from a widened cursor and maps unmet CSR rows to producer slots. ChangesGraph execution scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change lays groundwork for future in-graph early dispatch (recording-time qualification flags, sorted fanin rows, and a wider resumable wake-scan cursor) without turning on the actual early-dispatch execution path. The modified pieces are covered by new and updated unit tests, and a check for a possible attribute-timing regression across the recording-to-definition boundary did not find a live consumer affected. This looks safe to merge, with hardware validation still pending as noted by the authors. Sequence Diagram(s)sequenceDiagram
participant GraphDefinition
participant SchedulerState
participant ProducerSlot
GraphDefinition->>SchedulerState: provide fanin CSR and wake_scan_cursor
SchedulerState->>SchedulerState: scan unmet fanin rows backward
SchedulerState->>ProducerSlot: map CSR row to producer slot
ProducerSlot-->>SchedulerState: producer completion
SchedulerState->>GraphDefinition: advance wake_scan_cursor or queue consumer
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 51.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 8 files. (2 skipped: 2 unsupported.) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit sorts the fanin row, Comment |
4 tasks
ChaoZheng109
force-pushed
the
feat/hbg-graph-ed-prereq-g0g2
branch
2 times, most recently
from
September 8, 2026 06:37
eaee357 to
d3a4a90
Compare
Collaborator
Author
|
All three fixed in d3a4a90, and the A/B item 1 asked for is now run. Item 1 was the important one — the PR's own second UT is the counterexample, so the claim was wrong in exactly the place it mattered. 1. The backward-scan rationale — valid, and it was wrong in the load-bearing direction. "The builder emits a producer before its consumers" constrains a producer's task index, not its position within a row; a row holds the consumer's deduplicated operand order. I did both things you offered as alternatives, since with item 2 established the flip today lands entirely on rows whose rationale is heuristic. Device A/B, both arms on pinned die 12, 100 rounds, baseline =
4 improved, 4 regressed, spread -0.13% .. +0.44%, nothing near the 2% threshold; qwen moves +0.01%. So the direction flip on heuristic rows is device-neutral on this corpus — which is the honest claim, not that it helps. 2. Zero device coverage of the sort — valid, fixed, and instrumenting it found a mistake in my first attempt. I initially flagged both Row permutation stays covered by the UT rather than the scene test: 3. Re-verified after all of it: a2a3 hbg sim 12 passed / 7 skipped, a5 hbg 13 passed, C++ UTs 137/137. |
3 tasks
Collaborator
Author
|
Follow-up #2159 (per-GraphExecution compact state array; deletes the slot-resident |
zhusy54
reviewed
Sep 8, 2026
ChaoZheng109
force-pushed
the
feat/hbg-graph-ed-prereq-g0g2
branch
from
September 8, 2026 08:36
d3a4a90 to
9242c9e
Compare
…d a sorted CSR Early dispatch inside a Graph body needs two things the Definition did not carry, and one of them pays off on its own. Qualification moves to recording time. graph_fill_definition decides ED_FLAG_CANDIDATE and ED_FLAG_TRACKED against the Definition's own fanin CSR, using the same conjunction the top-level submit path applies to a payload's inline fanin: every producer carries allow_early_resolve, the task has no dispatch predicate, its shape is dispatchable, and its internal fanin is at least one. That last term excludes a body root, whose real gate is the outer shell's activation rather than this CSR. A body's structure is fixed by its Definition, so the verdict is decided once per recorded shape and every execution replays it. The caller's early-resolve intent now survives into the recording, which is the input being qualified, and is cleared on the way into the Definition instead — no in-graph task reaches the device carrying it. A hidden allocation is transparent to that conjunction. alloc_tensors marks its slot with allow_early_resolve at top level, because a buffer allocation's output is ready at creation and must never be the unflagged producer that disqualifies a consumer. That mark is applied after prepare_task, which the graph-recording branch returns before, so an allocation inside a body reached the recording unflagged and cost every consumer naming it the candidate verdict — even when its other producers all opted in. The recorded attrs now carry the mark. The device reads the flags for validation only; nothing propagates them to a slot yet, so this is inert until the publish chain is wired to in-graph tasks. A candidate's CSR row is then sorted by ascending producer index. A row holds the consumer's deduplicated operand order, which carries no relation to producer depth; sorting it makes the row's tail the deepest producer, since a producer is emitted before its consumers. Non-candidate rows keep their record order, so a body with no early-resolve flags is unchanged on the host side. graph_first_unmet_producer scans from the row's tail to make that bet, which finishes hw-native-sys#1924: the top-level classifier was flipped then, and its in-graph twin in the same file was not. It now resumes at a wake-scan cursor, and the drain routes a single-producer waiter straight to ready rather than rescanning a row whose only entry is the producer that just completed. Both reduce wake-list transfers and the CAS traffic they put on the lists. On an unsorted row the tail is the last-declared operand, so the direction is a heuristic there rather than an exact bet; the comments say so rather than claiming the sorted row's guarantee for every row. The graph_execution scene test flags `right` alongside `left` so `mul` qualifies on both its producers. That is what puts a candidate, a pair of tracked producers, and a sorted multi-entry fanin row on the device path, which no scene test produced before. The unit tests cover both halves: a recording whose consumer names a flagged task and an allocation pins the transparency above, and a hand-built execution pins the device side of the row — the tail-first scan, the cursor's resume and its 0xFFFF sentinel, the row-index-to-producer mapping, and the drain's single-producer fast path. Two mechanical consequences. The classifier returns the consumer's own CSR row index rather than a producer index, since that is what a cursor can mean; graph_producer_at maps it back. And wake_scan_cursor widens to uint16_t: an in-graph row is bounded by the in-graph task cap, not by CHIP_MAX_FANIN, so a byte would silently truncate on a large body. Its early-dispatch twin still indexes a payload's inline fanin alone and stays a byte. ChipTaskSlotState remains one cache line. InGraphTaskDefinition's two new bytes leave six bytes of interior padding that the per-field fill cannot reach. Nothing reads them, but a static_assert pins the struct's size so a later field's padding cost appears in the diff that adds it, and graph_fill_definition's header no longer claims every section is written in full. bind_graph_topology validates the named reserved byte too.
poursoul
approved these changes
Sep 8, 2026
4 tasks
ChaoZheng109
added a commit
to ChaoZheng109/simpler
that referenced
this pull request
Sep 9, 2026
Early dispatch reached only top-level tasks. It now also covers a Graph body's internal edges and the edge from an ordinary task into a Graph, which together leave one direction unhandled: a Graph as a producer, since a shell publishes no placement of its own for a consumer to bet on. **In-graph to in-graph.** The publish chain hw-native-sys#2095 built is reused, not forked. Nothing about the mechanism changes: a candidate hangs on its deepest unpublished producer, a producer that places its last logical block seals the chain, and detached waiters rescan and pre-stage. The cohorts differed only in where the fanin row lives and where the states do, and both were already made to match — the row by hw-native-sys#2144's sorted CSR, the states by hw-native-sys#2159's per-execution array — so three call sites take a cohort and the rest is shared. in_graph_execution_of names the cohort once, returning null for a GLOBAL task and for a GRAPH shell, which is a task of the run despite carrying a graph_context; complete_task routes on that same pair. Registration happens at materialization, where a body's tasks are already walked to hang each non-root on its first unmet producer. That point is single-owner per graph, so no peer can register the same task — a stronger guarantee than the top-level intake has. The completion path seals a tracked in-graph producer for the reason the global one does: COMPLETED >= PUBLISHED, so a producer that never publishes (a DUMMY, or one a predicate retired) still releases its waiters. Materialization also copies the Definition's ed_flags onto the slot, consuming the verdicts hw-native-sys#2144 recorded and left inert. **Ordinary task into a Graph.** A shell qualifies by the top-level rule minus the terms that describe dispatching to cores, since it has no predicate, no shape, and occupies no core: producers alone decide it. What its release does is stage the body's roots, each an ordinary AICore task with its own mask and blocks, gated exactly like any pre-staged task. They ring when the shell's real completion routes it through push_ready_routed, so the data dependency the shell stands for is still honoured — the shell's PUBLISHED buys placement, and only its COMPLETED launches. A root carries no host verdict, because qualification needs a producer to bet on and a root has none inside the body, so staging sets ED_FLAG_CANDIDATE on it first. push_ready_routed reads that flag as "this task may hold a staging claim, so check for a release", which is true of a root from that point on. Without it a staged root is gated and never rung, and the run ends in SIMPLER_ERROR_SCHEDULER_TIMEOUT. The graph_execution scene tests flag the seed task that feeds the Graph shells, so onboard CI exercises the ordinary-to-Graph edge rather than only the sim.
ChaoZheng109
added a commit
to ChaoZheng109/simpler
that referenced
this pull request
Sep 10, 2026
Early dispatch reached only top-level tasks. It now also covers a Graph body's internal edges and the edge from an ordinary task into a Graph, which together leave one direction unhandled: a Graph as a producer, since a shell publishes no placement of its own for a consumer to bet on. **In-graph to in-graph.** The publish chain hw-native-sys#2095 built is reused, not forked. Nothing about the mechanism changes: a candidate hangs on its deepest unpublished producer, a producer that places its last logical block seals the chain, and detached waiters rescan and pre-stage. The cohorts differed only in where the fanin row lives and where the states do, and both were already made to match — the row by hw-native-sys#2144's sorted CSR, the states by hw-native-sys#2159's per-execution array — so three call sites take a cohort and the rest is shared. in_graph_execution_of names the cohort once, returning null for a GLOBAL task and for a GRAPH shell, which is a task of the run despite carrying a graph_context; complete_task routes on that same pair. Registration happens at materialization, where a body's tasks are already walked to hang each non-root on its first unmet producer. That point is single-owner per graph, so no peer can register the same task — a stronger guarantee than the top-level intake has. The completion path seals a tracked in-graph producer for the reason the global one does: COMPLETED >= PUBLISHED, so a producer that never publishes (a DUMMY, or one a predicate retired) still releases its waiters. Materialization also copies the Definition's ed_flags onto the slot, consuming the verdicts hw-native-sys#2144 recorded and left inert. **Ordinary task into a Graph.** A shell qualifies by the top-level rule minus the terms that describe dispatching to cores, since it has no predicate, no shape, and occupies no core: producers alone decide it. What its release does is stage the body's roots, each an ordinary AICore task with its own mask and blocks, gated exactly like any pre-staged task. They ring when the shell's real completion routes it through push_ready_routed, so the data dependency the shell stands for is still honoured — the shell's PUBLISHED buys placement, and only its COMPLETED launches. A root carries no host verdict, because qualification needs a producer to bet on and a root has none inside the body, so staging sets ED_FLAG_CANDIDATE on it first. push_ready_routed reads that flag as "this task may hold a staging claim, so check for a release", which is true of a root from that point on. Without it a staged root is gated and never rung, and the run ends in SIMPLER_ERROR_SCHEDULER_TIMEOUT. The graph_execution scene tests flag the seed task that feeds the Graph shells, so onboard CI exercises the ordinary-to-Graph edge rather than only the sim.
ChaoZheng109
added a commit
to ChaoZheng109/simpler
that referenced
this pull request
Sep 10, 2026
Early dispatch reached only top-level tasks. It now also covers a Graph body's internal edges and the edge from an ordinary task into a Graph, which together leave one direction unhandled: a Graph as a producer, since a shell publishes no placement of its own for a consumer to bet on. **In-graph to in-graph.** The publish chain hw-native-sys#2095 built is reused, not forked. Nothing about the mechanism changes: a candidate hangs on its deepest unpublished producer, a producer that places its last logical block seals the chain, and detached waiters rescan and pre-stage. The cohorts differed only in where the fanin row lives and where the states do, and both were already made to match — the row by hw-native-sys#2144's sorted CSR, the states by hw-native-sys#2159's per-execution array — so three call sites take a cohort and the rest is shared. in_graph_execution_of names the cohort once, returning null for a GLOBAL task and for a GRAPH shell, which is a task of the run despite carrying a graph_context; complete_task routes on that same pair. Registration happens at materialization, where a body's tasks are already walked to hang each non-root on its first unmet producer. That point is single-owner per graph, so no peer can register the same task — a stronger guarantee than the top-level intake has. The completion path seals a tracked in-graph producer for the reason the global one does: COMPLETED >= PUBLISHED, so a producer that never publishes (a DUMMY, or one a predicate retired) still releases its waiters. Materialization also copies the Definition's ed_flags onto the slot, consuming the verdicts hw-native-sys#2144 recorded and left inert. **Ordinary task into a Graph.** A shell qualifies by the top-level rule minus the terms that describe dispatching to cores, since it has no predicate, no shape, and occupies no core: producers alone decide it. What its release does is stage the body's roots, each an ordinary AICore task with its own mask and blocks, gated exactly like any pre-staged task. They ring when the shell's real completion routes it through push_ready_routed, so the data dependency the shell stands for is still honoured — the shell's PUBLISHED buys placement, and only its COMPLETED launches. A root carries no host verdict, because qualification needs a producer to bet on and a root has none inside the body, so staging sets ED_FLAG_CANDIDATE on it first. push_ready_routed reads that flag as "this task may hold a staging claim, so check for a release", which is true of a root from that point on. Without it a staged root is gated and never rung, and the run ends in SIMPLER_ERROR_SCHEDULER_TIMEOUT. The graph_execution scene tests flag the seed task that feeds the Graph shells, so onboard CI exercises the ordinary-to-Graph edge rather than only the sim.
ChaoZheng109
added a commit
to ChaoZheng109/simpler
that referenced
this pull request
Sep 10, 2026
Early dispatch reached only top-level tasks. It now also covers a Graph body's internal edges and the edge from an ordinary task into a Graph, which together leave one direction unhandled: a Graph as a producer, since a shell publishes no placement of its own for a consumer to bet on. **In-graph to in-graph.** The publish chain hw-native-sys#2095 built is reused, not forked. Nothing about the mechanism changes: a candidate hangs on its deepest unpublished producer, a producer that places its last logical block seals the chain, and detached waiters rescan and pre-stage. The cohorts differed only in where the fanin row lives and where the states do, and both were already made to match — the row by hw-native-sys#2144's sorted CSR, the states by hw-native-sys#2159's per-execution array — so three call sites take a cohort and the rest is shared. in_graph_execution_of names the cohort once, returning null for a GLOBAL task and for a GRAPH shell, which is a task of the run despite carrying a graph_context; complete_task routes on that same pair. Registration happens at materialization, where a body's tasks are already walked to hang each non-root on its first unmet producer. That point is single-owner per graph, so no peer can register the same task — a stronger guarantee than the top-level intake has. The completion path seals a tracked in-graph producer for the reason the global one does: COMPLETED >= PUBLISHED, so a producer that never publishes (a DUMMY, or one a predicate retired) still releases its waiters. Materialization also copies the Definition's ed_flags onto the slot, consuming the verdicts hw-native-sys#2144 recorded and left inert. **Ordinary task into a Graph.** A shell qualifies by the top-level rule minus the terms that describe dispatching to cores, since it has no predicate, no shape, and occupies no core: producers alone decide it. What its release does is stage the body's roots, each an ordinary AICore task with its own mask and blocks, gated exactly like any pre-staged task. They ring when the shell's real completion routes it through push_ready_routed, so the data dependency the shell stands for is still honoured — the shell's PUBLISHED buys placement, and only its COMPLETED launches. A root carries no host verdict, because qualification needs a producer to bet on and a root has none inside the body, so staging sets ED_FLAG_CANDIDATE on it first. push_ready_routed reads that flag as "this task may hold a staging claim, so check for a release", which is true of a root from that point on. Without it a staged root is gated and never rung, and the run ends in SIMPLER_ERROR_SCHEDULER_TIMEOUT. The graph_execution scene tests flag the seed task that feeds the Graph shells, so onboard CI exercises the ordinary-to-Graph edge rather than only the sim.
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
3 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.
Summary
Early dispatch inside a Graph body needs two things the Definition did not carry, and one of them pays off on its own. Nothing here turns in-graph early dispatch on — that is the follow-up that wires the publish chain to in-graph tasks.
Qualification moves to recording time.
graph_fill_definitiondecidesED_FLAG_CANDIDATEandED_FLAG_TRACKEDagainst the Definition's own fanin CSR, applying the same conjunction the top-level submit path applies to a payload's inline fanin: every producer carriesallow_early_resolve, the task has no dispatch predicate, its shape is dispatchable, and its internal fanin is at least one. That last term excludes a body root, whose real gate is the outer shell's activation rather than this CSR. A body's structure is fixed by its Definition, so the verdict is decided once per recorded shape and every execution replays it — per-invocation cost is zero.The caller's early-resolve intent now survives into the recording, since that is the input being qualified, and is cleared on the way into the Definition instead. No in-graph task reaches the device carrying the bit, as before.
The device validates the flags but nothing propagates them to a slot, so this half is inert on device.
A candidate's CSR row is sorted by ascending producer index. The builder emits a producer before its consumers, so a sorted row's tail names its deepest producer — the entry a reverse scan should bet on. Non-candidate rows keep their record order, so a body with no early-resolve flags schedules exactly as it did before. That scoping is deliberate: an unconditional sort drifted flag-free workloads by ~0.5% in the qwen A/B done for #2095.
graph_first_unmet_producerscans backward to make that bet, which finishes #1924 — the top-level classifier was flipped then and its in-graph twin in the same file was not. It now resumes at a wake-scan cursor, and the drain routes a single-producer waiter straight to ready rather than rescanning a row whose only entry is the producer that just completed. Both reduce wake-list transfers and the CAS traffic they put on the lists.Two mechanical consequences worth calling out in review:
graph_producer_atmaps it back. All call sites move with it.wake_scan_cursorwidens touint16_t. An in-graph row is bounded by the in-graph task cap, not byCHIP_MAX_FANIN, so a byte would silently truncate on a large body. Its early-dispatch twin still indexes a payload's inline fanin alone and stays a byte, with astatic_asserton each.ChipTaskSlotStateremains one cache line.Testing
Re-verified after rebasing onto main now that #2130 has landed:
Simulation tests pass — a2a3
host_build_graph12 passed / 7 skipped, a5host_build_graph13 passedC++ unit tests — 137/137, including a new
test_hbg_graph_ed_qualification(both arches) that drives the real recording path to pin the Definition's verdicts and a candidate's sorted row, and asserts a non-candidate row keeps its record orderDevice A/B against
upstream/main, 100 rounds, 8 cases, both arms sequential on pinned die 12, baseline built in its own worktree venv. Every device delta lands in -0.13% .. +0.44%, nothing near the 2% threshold,qwen3_14b_decode+0.01%. Full table in this comment.The direction flip is therefore device-neutral on this corpus, which is the honest claim — not that it helps. Worth reading alongside the coverage note above: until this PR no scene test produced an in-graph candidate, so every row the flip lands on today is one whose backward-scan rationale is heuristic rather than exact.
Checks made while reviewing the change, recorded so they need not be re-derived: the fanout CSR build and the device-side topology validation read a fanin row as an unordered set, so sorting has no positional reader; the cursor is reset per materialization by
reset_for_reuse, so no stale cursor can survive into a later execution and shorten a scan.