Repository navigation
Refactor: retire the PTO2 prefix on the task-id type - #1966
Merged
Merged
Conversation
`PTO2TaskId` becomes `TaskId`. The name was free: its only other occurrence in the tree was this type's own docstring in `src/common/task_interface/task_id.h`, which already called it `TaskId` while the type was spelled `PTO2TaskId`. The external pto-isa headers do not use the name either. 2462 occurrences across 119 files, one identifier in one commit so no reader has to know both spellings (`.claude/rules/codestyle.md` rule 10). The shorter name re-flowed some aligned parameter lists and trailing comments, which is the rest of the diff. That header's docstring also loses "used across Runtime2": the `2` never denoted a version any reader can identify, and the encoding is shared by both runtimes, which is what the line now says. One identifier per change is deliberate. `PTO2TaskId` alone touches 119 files, so no batching of the remaining large identifiers stays under a reviewable size — the alternative is a single sweep across 464 files, which rule 10 rejects and which the bot reviewer skips outright. The remaining surface after this is 8651 occurrences over 150 identifiers; six of them (`PTO2Runtime`, `PTO2TaskSlotState`, `PTO2ReadyQueue`, `PTO2TensorMap`, and two smaller) collide with an existing type once stripped and need a considered name or a namespace rather than a prefix strip. `TaskId` is visible to orchestration sources outside this repo, which use it directly, so this lands without a compatibility alias by the same explicit decision as hw-native-sys#1963. Verified on this box: a2a3sim and a5sim scene sweeps green (59 and 52 cases), a2a3 onboard sweep green, cpput 117/117, pyut 1886 passed, clang-format and markdownlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 119 files, which is 19 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (119)
You can disable this status message by setting the 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 |
This was referenced Aug 24, 2026
ChaoZheng109
added a commit
to ChaoZheng109/simpler
that referenced
this pull request
Aug 24, 2026
…T edges Builds on the WAIT/RETAIN split (hw-native-sys#1806) to close Issue hw-native-sys#1375. When a consumer C has a direct edge P->C whose ordering is already implied by a two-hop path P->Q->C through another of C's own producers, the direct edge's WAIT flag is redundant and is cleared: - a creator edge (WAIT|RETAIN) becomes RETAIN-only, so P stays alive until C releases it — including past the end of P's scope — but no longer gates C's readiness (acceptance #2); - a modifier edge (WAIT-only) becomes DEP_NONE and drops out entirely (acceptance #1, the A->B->C + A->C diamond). reduce_wait_edges runs once per submit on the fully-built fanin, before it is flushed to the payload and wired. Q's fanin stores only a (ring, slot) pointer, not a generation, so slot reuse could make a walked producer P resolve to an unrelated task C also depends on. Two conditions together keep P's identity valid, and the reduction uses a covering producer Q only when both hold: - Q is still live (task_state < COMPLETED). on_task_release(Q), which drops Q's RETAIN pins, runs at Q's completion, not its reclaim, so a completed Q's fanin pointers may resolve to reused slots and Q is skipped. This loses no useful reduction: Q completing implies P completed (Q depends on P), so C's P->C WAIT is already satisfied and clearing it changes no readiness. - P->Q carries RETAIN. A live Q therefore still holds P's pin, so P is not consumed and its slot not rebound; a WAIT-only P->Q released P's pin at Q's wiring and gave no such guarantee. Reuse of P's slot requires all of P's pins released, and Q holds one until it completes, so a live Q with a RETAIN P->Q is the exact invariant proving the walked P is the same task Q recorded. The slot index is bounded on both sides before indexing the epoch array, only C's own fanin is modified, and the pass is bounded to inline-fitting fanin so the AICPU submit path stays O(count^2 * producer_fanin); denser fanins skip reduction. Readiness accounting now separates the two counts the split made distinct: fanin_actual_count stays the TOTAL edge count (for_each iteration and on_task_release walk every edge, including RETAIN-only ones), while a new fanin_wait_count carries the DEP_WAIT edge count after reduction and is the denominator the early-dispatch and completion paths compare dispatch_fanin against. fanin_wait_count occupies existing padding before the 64B-aligned predicate, so the payload layout is unchanged. The submit->wire pin is now released for any edge lacking DEP_RETAIN (ordering-only or reduction- dropped), replacing the PR1 assertion that every edge carried WAIT. Arbitrary-depth reduction is infeasible on the device orchestrator, which builds fanin incrementally and reuses ring slots and so has no reliable view past one hop; the reasoning and the deferral to a future host-resident DAG (hbg) are recorded in docs/investigations. deps.json continues to record the as-constructed (pre-reduction) edge set, and the dep_gen differential gate is unaffected since both replay passes drive the same construction; docs/dfx/dep-gen.md notes this and documents that the runtime OR-accumulates fanin flags per (pred, succ) pair, so consumers reconstructing aggregate dependency semantics must OR per-record flags across a pair's records. Mirrored identically across a2a3 and a5. Adds diamond-reduction, dropped-modifier, independent-producer, wait-only-covering-edge, completed-covering-producer, and RETAIN-only-lifetime acceptance tests per arch. The tests spell the task-id type `TaskId`, per its rename in hw-native-sys#1966. Closes hw-native-sys#1375
ChaoWao
added a commit
that referenced
this pull request
Aug 24, 2026
`PTO2OrchestrationConfig` -> `OrchestrationConfig`, `PTO2TaskSlotState` -> `ChipTaskSlotState`, `PTO2_SCOPE` -> `SIMPLER_SCOPE`, `PTO2Runtime` -> `RuntimeContext`, `PTO2_MAX_RING_DEPTH` -> `CHIP_MAX_RING_DEPTH`. 2671 occurrences across 323 files; every occurrence of each identifier moves together, so no reader has to know two spellings for one thing. Only the first is a plain prefix strip. Three of the five cannot take the bare name, and all three for the same reason: the host orchestrator under `src/common/hierarchical/` already holds it and models the same idea one tier up. `types.h` defines its own `TaskSlotState` out of `std::mutex` and `std::vector` where the renamed one is the chip runtime's 64-byte shared-memory slot; it defines its own `MAX_RING_DEPTH = 4`, which `worker_bind.h` exports to Python and which `docs/orchestrator.md` documents as deliberately equal to but distinct from the chip runtime's ring count; and `runtime.h` already has a `class Runtime` for AICPU/AICore handshake, whose docstring drew the boundary itself — "Task graph construction is handled by PTO2Runtime; this class only handles execution control." So `PTO2` was carrying real disambiguation in exactly the places where the chip runtime and the host orchestrator name one concept twice. Rule 13 decides which side keeps the bare name: the L3+ context does, and the chip runtime takes `Chip`, as `ChipWorker` / `ChipTensor` / `ChipTaskArgs` already do. The context struct is named for what its own docstring already called it. The scope macro is the one case answered by macro mechanics rather than by role. A bare `SCOPE` compiles — it is function-like, so it expands only at `SCOPE(`, and neither this tree nor the CANN headers nor pto-isa spells that token — but a macro obeys no namespace, so the name would claim an English word followed by `(` in every translation unit that includes `orchestration_api.h`, here and in every consumer repo, and it would stop being greppable: `git grep -w SCOPE` returns only `=== SCOPE STACK ===` comment banners today. `RT_` is unavailable because CANN owns it (`RT_ERROR_NONE`, consumed by `device_runner_helpers.cpp`), so the macro takes the `SIMPLER_` prefix the status codes took in #1963. Two clusters move as a unit rather than one identifier at a time, because splitting them would leave both spellings on adjacent lines of one declaration: `SIMPLER_SCOPE` with the `ScopeGuard` and `_SIMPLER_CONCATENATE` helpers that implement it, and `RuntimeContext` with the `RuntimeOps` table it carries as its first field, plus `RuntimeMode` and `RuntimeArenaLayout`. That last name had already converged: its only unprefixed occurrences were the gtest suite names in both `test_shared_memory.cpp` copies. Tier-A brand prose in the same files goes too: the `PTO Runtime2 - Main Interface` / `- Orchestrator Interface` / `- Main Implementation` banners, two `static_assert` messages, and the title and overview of both `tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.md` copies, which now name the runtime the way the `host_build_graph` copies already do. Those two copies had drifted apart on a `(PTO2)` heading suffix and agree again. Every one of these names is reachable from orchestration sources outside this repo — `SIMPLER_SCOPE` is typed directly in orchestration bodies and `RuntimeContext *rt` is the handle every entry point takes — so this lands without compatibility aliases by the same explicit decision as #1963 and #1966, with the matching renames and the merge order coordinated outside this change. 5846 `PTO2` occurrences remain across 319 files, led by `PTO2ResourceShape` (368), `PTO2TaskPayload` (309) and `PTO2SchedulerState` (305). Expect more of the collisions above rather than fewer: what is left is the chip-runtime structs closest to the host orchestrator's vocabulary, so each needs a name decided rather than a prefix deleted. Verified on an a2a3 box at the final commit: all four runtime variants build; a2a3sim 59 passed / 8 skipped; a5sim 52 passed; a2a3 onboard sweep 153 passed / 1 skipped; the quarantined SDMA step 2 passed; cpput 117/117 plus the hardware-labelled `test_comm_lifecycle`; pyut 1897 passed, with `test_second_child_failure_reaps_first` failing as the known flake in #1948 on a path this change leaves byte-identical to main. clang-format, ruff and markdownlint clean over all 323 changed files.
8 tasks done
ChaoWao
added a commit
that referenced
this pull request
Aug 25, 2026
Every `PTO2` name this repository reads is gone: 5804 occurrences before, 62 after (`git grep -Io -E "PTO2|pto2_" | wc -l`), across 315 files. 22 commits, one identifier or one coherent cluster each, so no reader ever has to know two spellings for one thing (`.claude/rules/codestyle.md` rule 10). Most of it is a prefix strip: `ResourceShape`, `TaskDescriptor`, `TaskPayload`, `SharedMemoryHeader` / `RingHeader` / `Handle`, `SchedulerState`, `OrchestratorState`, `TaskAllocator`, the `Fanin*` and `DepList*` types, the early-dispatch and sync-drain state machines with their fourteen enumerators, `SubtaskSlot`, `DispatchPayload`, `ScopeMode`, `LaunchSpec`, `OutputLayout`, the `sm_layout` namespace, and the constants that size each of them. Nine identifiers could not take the bare name, and the reason is worth stating because the check that finds it is not obvious: a target name has three independent owners to clear, and only the first is greppable here. - The **host orchestrator** under `src/common/hierarchical/` already declares `TensorMap`, `ReadyQueue`, `TaskState` (which is bound into Python), `Ring` / `HeapRing`, `AllocResult` and `MAX_SCOPE_DEPTH`. These are the same concepts one tier up, which is exactly where a prefix was doing real work. Rule 13 gives the bare name to the L3+ context, so the chip runtime's versions are `ChipTensorMap`, `ChipReadyQueue`, `ChipTaskState`, `ChipTaskRing` / `ChipHeapRing` / `ChipRingSet` and `CHIP_MAX_SCOPE_DEPTH`. - **External headers the same translation units include.** CANN defines `ALIGN_UP` in `acl/ops/acl_dvpp.h` and `acl/dvpp/hi_media_common.h`, so `PTO2_ALIGN_UP` becomes `CHIP_ALIGN_UP`. No grep of this tree can see that. - **Any declaration at all, if the name is an object-like macro**, because a macro rewrites its identifier textually instead of colliding where the compiler can report it. `#define HEAP_SIZE (256 * 1024 * 1024)` rewrote three cpput fixtures' `static constexpr uint64_t HEAP_SIZE = 4096;` into `static constexpr uint64_t (256 * 1024 * 1024) = 4096;`. So the macros in `runtime/runtime_types.h` keep `CHIP_` even where the bare name is free, while their `inline constexpr` siblings in `submit_types.h` take it. That third case also exposed a gap in the gate: `pip install -e .` builds the four runtime `.so` files and not the C++ unit tests, which are a separate CMake project. Both are compiled per commit now, and the commits that had chosen a bare macro name were replayed from the corrected plan rather than patched afterwards, so every commit here builds both trees. The `PTO2_RING_TASK_WINDOW` / `PTO2_RING_HEAP` / `PTO2_RING_DEP_POOL` environment variables are **removed rather than renamed**. `CallConfig.runtime_env` already carried the same three values per task, and per task is strictly more expressive: two tasks in one process can hold different ring sizes, which a process-wide export never allowed. So the whole parsing path goes -- `trim_copy`, `parse_uint_token`, `apply_env_ring_value[s]` and the scalar / comma-separated grammar in all four `host/runtime_maker.cpp` copies -- leaving per-task entries over the compile-time default with nothing in between, and taking two now-unused includes with it. An unrecognised environment variable is *ignored*, so a caller still exporting one of those names would run on the 256 MB default with its requested sizing nowhere in evidence, and the symptom would be a heap-exhaustion deadlock that reads as a runtime regression rather than a configuration change. `warn_on_retired_ring_env()` therefore names each retired variable that is set, once per bind, and points at `CallConfig.runtime_env`. It has no behavioral effect and is deletable once no caller sets them. This edge is real and the repo documents it itself: `.claude/skills/multi-repo-qwen-setup` instructed exporting `PTO2_RING_HEAP=4294967296` for batch-16 qwen prefill -- 4 GiB against that 256 MB default -- and the deepseek example records that pypto-lib's daily CI sizes its network the same way. Both now name `CallConfig.runtime_env` fields, and the skill says outright that a runner still exporting the old names deadlocks exactly as its own gotcha section describes. Removing an env knob is only half the change: every diagnostic that advertised one has to move with it, or the result is a hint naming a variable nothing reads. This change fixes three instances of precisely that defect, two of them pre-existing and found by reading every `PTO2_*` string: - `tools/benchmark_rounds.sh --serial-orch-sched` set `PTO2_SERIAL_ORCH_SCHED`, while the runtime reads `SIMPLER_TMR_SERIAL_ORCH_SCHED_ENABLE`. The serial arm ran with serial orch->sched still off, so the script measured the parallel path twice and reported the pair as a serial-versus-parallel comparison. Any measurement taken through that flag is void. - `error_hint(SIMPLER_ERROR_SCHEDULER_TIMEOUT)` told the user to raise `PTO2_SCHEDULER_TIMEOUT_MS`; the knob is `SIMPLER_SCHEDULER_TIMEOUT_MS`, which CI already sets by that name. Following the hint changed nothing and made a slow kernel look like a deadlock. - The ring knobs themselves: the device-side ring-buffer and orchestrator deadlock reports, the fanin-spill capacity report, five `error_hint()` strings, both capacity tables under `docs/troubleshooting/`, the `running-onboard` triage table and the runnable recipe in `docs/dfx/scope-stats.md` now name `runtime_env.ring_task_window` / `ring_heap` / `ring_dep_pool`. Two unit tests assert on that hint text and move with it, and `MULTI_RING.md` loses the two env tiers from its precedence ladder along with the section that was the only documentation of the comma-separated grammar. Three more references named things that do not exist under any spelling, which is how a stale comment stops being noise and becomes a lie: `pto2_task_slot()`, cited by `runtime_types.h` and `runtime_init.cpp` as the way to compute a slot (it is `local_id` masked by the window size, which the comment now says, including why the window must be a power of two); `pto2_run_runtime`, named five times in `docs/task-flow.md` as the L2 ABI edge (it is `simpler_run`); and `pto2_init_*` flags, listed in `docs/dynamic-linking.md` among the fields `SchedulerContext::deinit()` resets (it resets no such flag). The four shared-memory layout diagrams deferred by #1969 are correct now that every row -- `SharedMemoryHeader`, `TaskDescriptor[]`, `TaskPayload[]`, `ChipTaskSlotState[]` -- names a type that exists under exactly that spelling; the two multi-ring boxes widen by four columns to fit. Rule 10 previously described a 6.6k-occurrence backlog across 367 files; it now records that the retirement is complete, so a new `PTO2` match is a defect rather than backlog for a regression to hide behind. What remains is 62 occurrences in 17 files, none of them read here: this rule file's own illustrations of the banned spelling, the retired-name list inside the warning, prose recording that these names are gone, and `PTO2_MANUAL_MAX_SEQ` -- a pypto-lib knob two example READMEs document and this repo never reads. These names are reachable from orchestration sources outside this repo, so this lands without compatibility aliases by the same explicit decision as #1963, #1966 and #1969; the matching renames and the merge order are coordinated outside this change. One commit here is not about naming at all. The clang-tidy pre-commit hook was failing on this branch, and because every gated CI job declares `pre-commit` in `needs:`, that one failure skipped the entire test matrix rather than failing a single job. #1845 added `include("${SIMPLER_CMAKE_DIR}/host_log_sources.cmake")` to six platform `CMakeLists.txt` without an install rule to match, so `simpler_setup/_assets/cmake/` never shipped and any runtime cmake configure from a wheel install died on that include. All four files under `cmake/` are reachable from a platform configure, so the whole directory ships now; installing only the file the error named would have failed at the next `include()`. It stayed hidden because the hook only configures on a build-cache miss and `tools/verify_packaging.sh` checks entry points rather than the asset tree. Verified by building the wheel both ways: `pip wheel --no-build-isolation --no-deps .` produces zero `simpler_setup/_assets/cmake/*` entries before the change and all four after. Verified on an a2a3 box at the final commit: all four runtime variants and the cpput targets build at every commit; a2a3sim 59 passed / 8 skipped; a5sim 52 passed; a2a3 onboard sweep 153 passed / 1 skipped; the quarantined SDMA step 2 passed; cpput 119/119 plus the hardware-labelled `test_comm_lifecycle`; pyut 1908 passed, 14 skipped. Exporting all three retired ring variables makes an a2a3sim scene test log all three warnings and still pass, which is what proves both that the warning fires and that a `PTO2_RING_TASK_WINDOW=64` is now inert. clang-format, markdownlint, ruff and `bash -n` clean across the diff.
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
PTO2TaskId→TaskId. 2462 occurrences across 119 files, one identifier in one commit so no reader has to know both spellings (.claude/rules/codestyle.mdrule 10).The name was free, and the tree already wanted it: the only other occurrence of
TaskIdwas this type's own docstring insrc/common/task_interface/task_id.h, which called itTaskIdwhile the type was spelledPTO2TaskId. The external pto-isa headers do not use the name either.That docstring also loses "used across Runtime2" — the
2never denoted a version any reader can identify, and the encoding is shared by both runtimes, which is what the line now says.The shorter name re-flowed some aligned parameter lists and trailing comments; that is the rest of the diff.
Why one identifier per PR
PTO2TaskIdalone touches 119 files, so there is no batching of the remaining large identifiers that stays under a reviewable size — measured per identifier:PTO2TaskIdPTO2OrchestrationConfigPTO2TaskSlotStatePTO2_SCOPEPTO2_MAX_RING_DEPTHPTO2RuntimeThe alternative is one sweep over 464 files, which rule 10 rejects outright and which the bot reviewer skips (it skipped #1963 at 475 files). Splitting by identifier at least keeps each diff mechanical and single-purpose.
Remaining after this: 8653 occurrences over 150 identifiers. Six of them —
PTO2Runtime,PTO2TaskSlotState,PTO2ReadyQueue,PTO2TensorMapand two smaller — collide with an existing type once stripped, so they need a considered name or a namespace rather than a prefix strip. That is the disambiguation thePTO2prefix was actually providing, and rule 9 asks for "clear names or anamespace" in its place.Cross-repo
TaskIdis used directly by orchestration sources outside this repo, so this lands without a compatibility alias by the same explicit decision as #1963. Those repos need the matching rename and merge order matters.Testing
a2a3simscene sweep — 59 cases, 0 failuresa5simscene sweep — 52 cases, 0 failuresa2a3onboard sweep (-m "not sdma" --exclude-level 4) — 149 passed, 1 skippedclang-format,ruff,markdownlint-cli2clean