Repository navigation
Remove documentation that describes things that no longer exist - #2017
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request deletes Changes
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only removes an unreferenced scratch document and does not affect code, builds, tests, or shipped behavior; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. 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. Comment |
Three kinds of stale documentation, all of it naming something the tree does not contain. `doc-consistency.md` §3: when a referent is gone, the reference goes too. **`problems.md`, deleted.** A personal working audit of `host_build_graph` — findings, their status, and notes on what to look at next. It was never meant to be here. It arrived in hw-native-sys#1974 through a `git add -A` that swept it up with that change's 958 files, and hw-native-sys#2012 then updated it rather than noticing it. Nothing references it (`git grep problems.md` is empty), it is absent from `mkdocs.yml`, and `wheel.packages` is `["simpler_setup", "python/simpler"]` so it never shipped. Its closed items name the PRs that closed them, each of which carries the reasoning in its own commit message. **`last_task_alive`, in hbg's `RUNTIME_LOGIC.md`.** The paragraph gives four reasons `SchedulerState` holds no per-run content, and the third was "hbg never advances `last_task_alive`". That was accurate when written — hbg inherited the field from `tensormap_and_ringbuffer` and did not advance it — but hw-native-sys#1837 deleted the field with the rest of the reclamation paths, for exactly the reason the sentence gives, and hw-native-sys#2004 then removed the ring that held it. The clause now names the one thing in that list that does not exist. It is replaced by the live fact from the same runtime: polling reserves no wiring or dependency pool, because readiness comes from the task table's `completion_flags`. **`ring 0`, in hbg's `SCALAR_DATA_ACCESS.md`.** Ownership validation was described as checking that "the task ID ... carries ring 0, the only ring HBG places tasks on". hbg has had no ring since hw-native-sys#2004 (`git grep 'rings\['` over its tree is empty), and since hw-native-sys#2012 the check is on the id *space*, not a ring index. The bullet now says what the code does and why a `GRAPH_NODE` id fails it. **`pto_runtime2_init`, in three `scheduler.h` files.** A comment credited it with reserving and wiring the early-dispatch queues. `git grep pto_runtime2_init` returned only those three comments — the symbol exists nowhere. The work is done by `SchedulerState::init_data_from_layout`, which the comment now names. Only three of the four runtime trees carried the line; `src/a5/runtime/tensormap_and_ringbuffer` words that member differently. **`run_from_blob`, in `docs/buffer-abi.md`.** A present-tense table row told the chip leaf to "hand the POD blob to `run_from_blob`", a name that has never existed (introduced with the row in hw-native-sys#1599). The row now names the three functions that do the work — `read_args_from_blob`, `ImportRegistry.materialize_args`, `_submit_chip_run_materialized` — matching `worker.py`'s `submit_frame`. Deliberately untouched: `tensormap_and_ringbuffer`'s ring documentation. `current_task_index`, `task_window_mask`, `advance_lock` and `last_task_alive` are all live there (162 uses across 20 non-doc files); hw-native-sys#2004 dropped the ring from `host_build_graph` only, and said so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
force-pushed
the
remove-problems-scratch-file
branch
from
August 26, 2026 01:32
285df3d to
fdc4d69
Compare
ChaoWao
changed the title
poursoul
added a commit
that referenced
this pull request
Aug 27, 2026
… its ring names (#2030) Four changes to `host_build_graph`. The first is a live out-of-bounds write; the rest close the gap between what the runtime does and what its own comments claim. **The out-of-bounds write.** A Graph recording's hazard map holds `GRAPH_MAX_NODES` task chains and indexes them by a producer's low id field directly, but a recorded node was minted in the RING space as `start_local_task_id + node_index` — the allocator's own numbering. Any Graph beginning after the run's first `GRAPH_MAX_NODES` tasks, which is the ordinary shape for a decode workload, therefore registered its nodes past the last chain and wrote outside `task_entry_heads`. ASAN puts it at `link_entry` → `insert` → `register_task_outputs` → `graph_record_submit_node`, on the 8192-byte region `graph_recording_init_tensor_map` allocates for `max_tasks = GRAPH_MAX_NODES = 1024`. A recorded node now takes an `IN_GRAPH` id whose low field is the node index alone, so the key is in range however far into a run the Graph begins. The recording baseline that existed only to tell a node from a pre-Graph task by numeric range is gone: the two now differ by id space, decided at the mint rather than derived by subtraction. Two consequences beyond the write itself — a ring id the main thread allocates while a recorder runs can no longer land inside a node's index range and be wired as an internal fanin, and a node id that escapes its Graph and is later declared as an explicit dependency now trips `append_fanin_or_fail`'s id-space guard instead of resolving to an unrelated task slot. `link_entry` and `remove_from_task` gained a `debug_assert` on the chain index so a future caller inserting under the wrong id space fails where it happens. **The id spaces.** `TaskIdSpace::RING` came in with #2012, which gave hbg its own `TaskId` encoding but kept the older ring vocabulary for the two spaces — and hbg has had no ring since #2004 (`git grep 'rings\['` over its tree is empty), so the header's own comment had to apologize for the name. There is likewise no separate "node" concept to name: everything the runtime schedules is a task, and the only question the high bits answer is whether a task belongs to a Graph. TaskIdSpace::RING -> TaskIdSpace::GLOBAL TaskIdSpace::GRAPH_NODE -> TaskIdSpace::IN_GRAPH make_ring_task -> make_global_task is_ring_task -> is_global_task make_graph_node -> make_in_graph_task GRAPH_NODE_INDEX_BITS -> IN_GRAPH_TASK_INDEX_BITS `TaskIdSpace` keeps its name: the field holds which namespace an id belongs to, and "type" would collide conceptually with `TaskKind`. The deeper `node` vocabulary is untouched — `GraphNodeStorage`, `GraphNodeDefinition`, `graph_node_index`, `GRAPH_MAX_NODES`, `node_count` and `node_at` are ~350 occurrences across host, device and the Definition wire structs, and `TaskKind::GRAPH_NODE` is not a rename at all, since it also discriminates what `graph_context` points at. **The ring vocabulary in comments and docs.** Renaming the identifiers left the surrounding prose still describing tasks as living on a ring, so one concept stayed spelled two ways. Six of those comments were not stale wording but descriptions of mechanisms the runtime does not have, which is the part worth reading: - `TaskPayload`'s layout comment promised that large fanins "spill into a per-ring ring buffer slice". Fanin is always inline and hard-capped at `CHIP_MAX_FANIN`, which `append_fanin_or_fail`'s own fatal states. - `reserve_layout` and `wire_arena_pointers` claimed to declare and wire per-ring `dep_pool` regions. Polling has no `dep_pool`, as a comment 700 lines above them already says. - `TaskDescriptor` placed itself in a "ring buffer"; it lives in the task table. - `TaskPayload::init` and the predicate write in `submit_task_common` attributed uninitialized payload storage to slot reuse. hbg never reuses a slot: the storage is raw because shared memory is not zero-filled, and that is what the surrounding code depends on. The rest is vocabulary. `per-ring completed_watermark` drops a modifier that distinguishes nothing, there being one watermark. `the ring path` becomes `the ordinary path`, which is what `GRAPH_EXECUTION.md` and eight existing comments already call it — one of them the line directly below a `ring path` mention. hbg's `SCALAR_DATA_ACCESS.md` moves for the second time in this sequence: #2017 corrected its "ring 0" claim to name the `RING` id space, and that name is what this change retires. Untouched, because each is accurate or externally consumed: the ring names that describe `tensormap_and_ringbuffer`, the mailbox and ready-queue ring buffers, the doorbell verb (`ring_one_doorbell`, `maybe_rendezvous_ring`, ...), the `runtime_env.ring_*` knobs, and the `TASK ring=%d` stall-log field. **`FaninBuilder`, dropped.** It had shrunk to four fields and one method, two of those fields copies of `payload.fanin_count` and `payload.fanin_data()`, with STEP 6 copying the count back to the payload at the end of every submit. `append_fanin_or_fail` now takes the consumer payload's fanin region and its count directly, so the count accumulates in place and the copy-back is gone; `mark_seen` becomes the free function `fanin_mark_seen`. The region is still resolved once per task rather than per producer. `submit_task_common` zeroes `payload.fanin_count` before the appends, which is this device-read field's init-on-write point since hbg does not zero-fill the task table; `graph_submit_outer` needs no zeroing because `graph_reset_outer_payload` already does it. No functional change. Also converts `dep_compute.h` to `#pragma once`: its guard macro named `tensormap_and_ringbuffer` and carried the retired `PTO` prefix, and the a5 copy was already converted. Testing: - `ctest` on `tests/ut/cpp`: 121/121. - ASAN build of `tests/ut/cpp`: the two `link_entry` overflows reported before the fix are both gone. CI does not cover this — `sanitizers.yml` runs sim scene tests, not cpput. - `test_hbg_graph_recording_bounds` is new and fails against the pre-fix tree with `task_local_id(node_id)` at 1024 against a 1024-chain map. It runs on both arch trees, since the fix is mirrored in both. - `test_hbg_tensormap`'s slot-aliasing case rested on a mask hbg does not have — its own inserts ran one chain past the fixture's dimension. It now reads `task_entry_heads` to pin the invariant that holds: a local id is its own chain index. An entry count cannot observe that, which is why the earlier `valid_count()` assertion did not. - Scene tests on `a2a3sim` and `a5sim` with `--manual include`, plus `host_build_graph_validation` including `graph_node_dependency` — the standing barrier for "a Graph-internal id used as an explicit dependency is a fatal".
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
1 participant
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
Documentation that names things the tree does not contain.
doc-consistency.md§3: when a referent is gone, the reference goes too.problems.md— deletedA personal working audit of
host_build_graph. It was never meant to be here: it arrived in #1974 through agit add -Athat swept it up with that change's 958 files, and #2012 then updated it rather than noticing. Both mine.git grep problems.mdmkdocs.ymlwheel.packages=["simpler_setup", "python/simpler"]git log --followIts closed items name the PRs that closed them (#1963, #1965, #1974, #2004, #2012), each of which carries the reasoning in its own commit message.
last_task_alive— hbg'sRUNTIME_LOGIC.mdThe paragraph gives four reasons
SchedulerStateholds no per-run content; the third was "hbg never advanceslast_task_alive".That was accurate when written — hbg inherited the field from
tensormap_and_ringbufferand did not advance it. Then #1837 deleted the field along with the rest of the reclamation paths, for exactly the reason the sentence gives, and #2004 removed the ring that held it. Neither updated this line, so the list now includes the one item that does not exist:Replaced with the live fact from the same runtime: polling reserves no wiring or dependency pool, because readiness comes from the task table's
completion_flags.ring 0— hbg'sSCALAR_DATA_ACCESS.mdOwnership validation was described as checking that "the task ID … carries ring 0, the only ring HBG places tasks on". hbg has had no ring since #2004 (
git grep 'rings\['over its tree is empty), and since #2012 the check is on the id space. The bullet now says what the code does and why aGRAPH_NODEid fails it — this one is my own #2012's fault,doc-consistency.md§1 asks for a grep after changing a guard and I did not do it.pto_runtime2_init— threescheduler.hfilesA comment credited it with reserving and wiring the early-dispatch queues.
git grep pto_runtime2_initreturned only those three comments — the symbol exists nowhere. The work is done bySchedulerState::init_data_from_layout, which the comment now names.Only three of the four runtime trees carried the line;
src/a5/runtime/tensormap_and_ringbufferwords that member differently, so the near-duplicates had drifted here too.run_from_blob—docs/buffer-abi.mdA present-tense table row told the chip leaf to "hand the POD blob to
run_from_blob" — a name that has never existed, introduced with the row in #1599. The row now names the three functions that do the work, matchingworker.py'ssubmit_frame:read_args_from_blob→ImportRegistry.materialize_args→_submit_chip_run_materialized.Deliberately untouched
tensormap_and_ringbuffer's ring documentation.current_task_index,task_window_mask,advance_lockandlast_task_aliveare all live there — 162 uses across 20 non-doc files. #2004 dropped the ring fromhost_build_graphonly, and its own message says "while leaving the reclaiming T&R runtime unchanged". Those docs are correct.Testing
Markdown plus C++ comments. The three
scheduler.hfiles are.hundersrc/, sodetect-changesclassifies this as code and runs the full matrix — verified locally that it compiles and that nothing moved:ctestcpput — 119/119markdownlint-cli2on all five changed docs — 0 errors🤖 Generated with Claude Code