tensormap_and_ringbuffer: split fanin edges into orthogonal WAIT/RETAIN flags (#1375) - #1806
Merged
ChaoZheng109 merged 1 commit intoAug 13, 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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe runtime now separates dependency readiness ( ChangesDependency semantics and orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/ut/cpp/a2a3/test_wiring.cpp (1)
1087-1125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNeither wiring test suite covers a
DEP_RETAIN-only edge. Both suites testDEP_WAITandDEP_WAIT | DEP_RETAINonly, so the retention-only contract required by issue#1375stays unverified in both runtimes.
tests/ut/cpp/a2a3/test_wiring.cpp#L1087-L1125: add a test with aDEP_RETAIN-only edge that asserts nofanout_headlink, no readiness contribution, no pin release at wiring, and a pin release aton_task_release.tests/ut/cpp/a5/test_wiring.cpp#L476-L520: add the mirrored test so the a2a3 and a5 suites stay aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/cpp/a2a3/test_wiring.cpp` around lines 1087 - 1125, Add mirrored tests for a DEP_RETAIN-only edge in tests/ut/cpp/a2a3/test_wiring.cpp:1087-1125 and tests/ut/cpp/a5/test_wiring.cpp:476-520. Using the existing wiring test patterns, assert that the edge creates no fanout_head link, contributes nothing to readiness, does not release its pin during wiring, and releases the pin exactly once through sched.on_task_release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dfx/dep-gen.md`:
- Line 217: Update the `flags` schema documentation to state that explicit edges
default to `["wait","retain"]`, while allowing `["wait"]` for explicit edges
created through the ordering-only `CoreTaskArgsWithDeps::add_dep_wait()` API;
retain the existing descriptions for creator and tensormap edges.
In `@src/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.md`:
- Line 439: Update the fanin metadata and wiring descriptions in
src/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.md:439-439 and
src/a5/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.md:440-440 to
distinguish WAIT-only, RETAIN-only, and combined producer edges: only DEP_WAIT
contributes to fanin_count and fanout notifications, DEP_WAIT-only pins are
released after wiring, and DEP_RETAIN pins are released when the consumer
completes. Keep both documents in parity.
In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cpp`:
- Around line 457-486: Store fanin_builder.wait_count in the payload as the
DEP_WAIT edge count, then replace fanin_actual_count with that value for
early-dispatch threshold checks at the code paths corresponding to lines 497,
988, and 1102. Keep DEP_RETAIN-only edges excluded so thresholds align with
dispatch_fanin.
In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_types.h`:
- Around line 452-477: Update the documentation comment for
set_dependencies_with_kinds to state that set_dependencies() uses the DEP_WAIT |
DEP_RETAIN default for every dependency, matching explicit_dep_kind() and the
actual API behavior. Leave the implementation unchanged.
In `@src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cpp`:
- Around line 452-481: Align early-dispatch counting in this fanin handling with
the denominator used at the comparisons against payload->fanin_actual_count:
either count all fanin edges consistently or use a matching DEP_WAIT-only
denominator. Apply the same correction to the corresponding a2a3 mirror and
preserve correct behavior when DEP_RETAIN-only edges are present, including the
transitive-reduction follow-up.
In `@src/a5/runtime/tensormap_and_ringbuffer/runtime/pto_types.h`:
- Around line 453-459: Correct the set_dependencies() reference in the
documentation for the per-dependency DepFlags overload so it states that the
default is DEP_WAIT | DEP_RETAIN, not “all-RETAIN.”
- Around line 461-478: Update set_dependencies_with_kinds to validate every
DepFlags value before storing the dependency arrays, rejecting any bits outside
the supported mask 0x3 via set_error. Ensure validation occurs before mutating
explicit_deps_, explicit_dep_kinds_, or explicit_dep_count_, and preserve the
existing zero-count and null-input behavior.
---
Nitpick comments:
In `@tests/ut/cpp/a2a3/test_wiring.cpp`:
- Around line 1087-1125: Add mirrored tests for a DEP_RETAIN-only edge in
tests/ut/cpp/a2a3/test_wiring.cpp:1087-1125 and
tests/ut/cpp/a5/test_wiring.cpp:476-520. Using the existing wiring test
patterns, assert that the edge creates no fanout_head link, contributes nothing
to readiness, does not release its pin during wiring, and releases the pin
exactly once through sched.on_task_release.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92f5031d-146c-4c6f-9d7b-b458fd4b382b
📒 Files selected for processing (25)
docs/dfx/dep-gen.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/host/dep_gen_replay.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/orchestration/pto_arg_with_deps.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_dep_compute.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.hsrc/a5/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a5/runtime/tensormap_and_ringbuffer/host/dep_gen_replay.cppsrc/a5/runtime/tensormap_and_ringbuffer/orchestration/pto_arg_with_deps.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_dep_compute.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_ring_buffer.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_types.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.htests/ut/cpp/a2a3/test_fanin_pool.cpptests/ut/cpp/a2a3/test_orchestrator_fanin.cpptests/ut/cpp/a2a3/test_wiring.cpptests/ut/cpp/a5/test_fanin_pool.cpptests/ut/cpp/a5/test_orchestrator_fanin.cpptests/ut/cpp/a5/test_wiring.cpp
ChaoZheng109
force-pushed
the
feat/issue-1375-split-wait-retain-deps
branch
4 times, most recently
from
August 13, 2026 11:52
961de27 to
4d8a1d2
Compare
…IN flags Fixes hw-native-sys#1375 A fanin edge previously carried two coupled semantics at once: readiness (the consumer waits for the producer) and lifetime (the producer's slot and packed output stay alive until the consumer releases them). Coupling them means a modifier producer is retained for the consumer's whole lifetime even though only ordering is needed, and it blocks the transitive-reduction work that must drop a redundant ordering edge while keeping a required lifetime reference. Introduce DepFlags { DEP_WAIT, DEP_RETAIN } and carry the flags per edge: - Storage: pack the flags into the low 2 bits of the producer slot pointer, for both the inline fanin array and the spill pool. PTO2TaskSlotState is alignas(64), so the low bits are free and sizeof is unchanged. This unifies the inline and spill representations and needs no extra payload field. - Construction: a creator (owner_task_id) edge is DEP_WAIT|DEP_RETAIN; a tensormap-modifier edge is DEP_WAIT. Explicit deps default to DEP_WAIT|DEP_RETAIN (conservative); CoreTaskArgsWithDeps gains add_dep_wait() for ordering-only deps. When a producer is reached for several reasons, the flags are OR-accumulated on dedup rather than first-wins. - Accounting: readiness (fanin_count, fanout_head linkage, dep_pool reservation) counts DEP_WAIT edges only. A DEP_WAIT edge without DEP_RETAIN releases its submit->wire pin at wiring (and on the all-completed fast path), so a modifier producer can be CONSUMED without waiting for this consumer. on_task_release releases DEP_RETAIN edges only. - DFX: dep_gen replay threads DepFlags through the oracle, records them per edge in deps.json, and the differential gate now compares the (producer -> flags) mapping instead of the producer-id set alone. Storing the flags inside the edge pointers (written when the builder flushes to the payload) also avoids the separate-mask/init-order hazard, since there is no field for payload.init() to zero after the fact. Lands identically in a2a3 and a5. No RETAIN-only edge is produced yet; the transitive-reduction pass that creates them is a follow-up. Tests: fanin-pool flag round-trip across inline and spill; a wiring regression asserting an ordering-only producer is released at wiring while a retention producer is held until on_task_release, including a spilled RETAIN edge; the duplicate-explicit-dep test now checks the folded flags. PTO2FaninSpillEntry::set/add_flags mask the flags to the tag bits so a malformed DepFlags value can never corrupt the packed slot pointer, and an assert pins the PR1 invariant that every fanin edge carries DEP_WAIT (so fanin_actual_count is the early-dispatch WAIT denominator). The fanin edge stays trivially default-constructible (no in-class initializer), so the payload's inline edge array and the per-submit builder array are not zeroed on the orchestrator hot path; a static_assert pins the slot-pointer alignment the tag bits rely on. The payload field is renamed fanin_inline_edges to match its packed-edge contents, or_flags_into_existing folds through a single index helper and fatals rather than silently degrading if a deduped producer is missing, and the early-dispatch WAIT-denominator invariant uses always_assert so it survives release builds. deps.json records explicit edges as wait+retain (the DepGenRecord carries no per-dep kind) and the retention-safety invariant behind the modifier=WAIT downgrade is documented at compute_task_fanin Step B.
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
Fixes #1375 (PR1 of 2 — the WAIT/RETAIN infrastructure).
In
tensormap_and_ringbuffer, one fanin edge coupled two semantics: WAIT(the consumer waits for the producer) and RETAIN (the producer's slot +
packed output stay alive until the consumer releases them). Coupling them
retains modifier producers for the consumer's whole lifetime even though only
ordering is needed, and blocks the transitive-reduction work that must drop a
redundant ordering edge while keeping a required lifetime reference.
This PR makes the two semantics orthogonal per edge and splits the accounting.
It lands identically in a2a3 and a5.
What changed
DepFlags { DEP_WAIT, DEP_RETAIN }inpto_types.h.pointer, for both the inline fanin array and the spill pool
(
PTO2TaskSlotStateisalignas(64)).sizeofunchanged; no new payloadfield — which also removes the separate-mask/
init()-order hazard thereference prototype had.
owner_task_id) →WAIT|RETAIN; tensormapmodifier →
WAIT; explicit deps defaultWAIT|RETAIN, with a newCoreTaskArgsWithDeps::add_dep_wait()for ordering-only deps. Duplicateproducers OR-accumulate their flags on dedup (no first-wins ordering).
fanin_count,fanout_headlinkage, dep_poolreservation) counts
DEP_WAITedges only. AWAIT-without-RETAINedgereleases its submit→wire pin at wiring (and on the all-completed fast
path), so a modifier producer can be CONSUMED without waiting for this
consumer.
on_task_releasereleasesDEP_RETAINedges only.DepFlagsthrough the oracle, records"flags"per edge indeps.json, and the differential gate now compares the(producer → flags)mapping instead of the producer-id set alone.No
RETAIN-only edge is produced yet — nothing generates one until thetransitive-reduction pass, which is the follow-up PR2 (device-side 1-hop,
per the discussion on #1375). This PR is the enabling representation +
accounting it builds on, plus the modifier early-consume win.
Tests
DepFlagsround-trip across inlineand spill; wiring regressions (ordering-only producer released at wiring
vs. retention held until
on_task_release, including a spilledRETAINedge); orchestrator-level tests for a WAIT-only explicit dep, OR-fold of two
discovery kinds on one producer (inline and spill region, asserting the
spilled edge's folded flags), and the all-completed fast-path pin release.
dfx/dep_genon a2a3sim + a5sim (exercises theflag-aware replay gate and
deps.jsonflags end-to-end);dummy_taskandmixed_exampleon both sims.tests/st/a2a3/tensormap_and_ringbufferscene-test suite — 33 passed; a30-iteration stress loop over the dependency-heavy
mixed_example+alternating_matmul_add(exercising the modifier WAIT-only early-release pathand slot reuse) — all passed, no
507018/ hang / wrong result. This isthe concurrency validation for the one real behavior change (early pin release
makes
COMPLETED → CONSUMED → reset_for_reusea common path).Notes for review
host_build_graphhas its ownappend_fanin_or_failand is otherwiseuntouched; the only hbg change is one comment line, fixing a now-dangling
fanin_inline_slot_statesreference to hbg's actualfanin_local_idsafterthe payload field rename.
or_flags_into_existingusesalways_assertwhen a deduped producer is somehow absent from the builder (a seen-set/builder
desync — currently unreachable). This is a loud device failure rather than a
silent weak-semantics degrade; flagging the "crash-not-continue" choice for
confirmation. Can switch to
report_fatalif preferred.