Repository navigation
host_build_graph: upload the bind image once, and only the bytes the device reads - #1947
Merged
Merged
Conversation
…device reads The bind stage made 46 host-to-device transfers per bind and reserved 361 MB of device memory, most of it for data the device never reads. Both come from the same habit: every structure was shipped at the width of its type and copied on its own. **One copy instead of forty-six.** A slot's payload and descriptor are named by a self-relative offset — an int32 delta from the field's own address — so the whole ring image survives a single memcpy with no pointer fix-up, which retires the host-to-device relocation pass. The Graph submissions go up as one block rather than one allocation and one copy each, and both the shared-memory image and that submission block become tails of the runtime arena's own region, so they ride the arena's copy. What is left is one arena copy plus one retained Definition object. **Only the bytes the device reads.** The arena's host-only zone is dep-computation scratch no device code touches, so it is no longer reserved on the device at all. The shared-memory image ships pitched to the submitted task count rather than the ring capacity — 47 slots of 16,384 on qwen3-14b decode — and the device-side reservation follows the same pitch, taking it from 81,412,352 bytes to 77,440. **Strided by what a bind actually holds, not by the type.** `PTO2TaskPayload`'s tensor array and `GraphNodeStorage`'s payload move last so each can be truncated, and the shipped payload array and the Graph execution storage are then strided by the widest task and the widest node in the bind. `sizeof(GraphNodeStorage)` is not a power of two, so indexing already compiled to a multiply; a runtime stride changes the operand from an immediate to a register and adds no indirection on the dispatch path. The int32 delta carries two bounds with it. `set()` leaves the field unbound rather than storing a truncated delta, since unbound is the value every consumer already tests for and a truncated one names unrelated memory; `attach_populated` rejects an image whose end exceeds the bound, next to the live-count and stride bounds it already checks. On the host side `compact_live_image` asserts those same two bounds, where exceeding them would read past the mirror's segments and ship a corrupt image with nothing to say so. Three defects fall out of the same reading, and none is an optimization: - A Graph outer task's slot was not reset as it was claimed. It was only accidentally correct: the host mirror is a fresh `new uint8_t[]`, which the kernel hands back zero-filled, so a stale slot never appeared. A reused slot or a reused mirror would have read the previous run's state. Covered by a test that fails without the reset. - A latched orchestration fatal did not stop the upload. A heap or tensormap exhaustion drops tasks and a fanin overflow drops edges, and the truncated graph was shipped anyway; the error code only reached the host after the device had launched and timed out, so the reported failure was a scheduler timeout with the real cause behind it. Checking the same word before the upload costs one relaxed load. The bind then fails with the latched code itself — mapped by `runtime_status_from_error_codes`, the same mapping the run path uses, since a caller cannot tell which of the two noticed — and names it through `LOG_RUNTIME_FAILURE` at the point of detection, so the diagnosis arrives before the launch rather than after a round-trip. - The execution storage reserved one stride per entry, while materialization placement-news a whole `GraphNodeStorage` into each, so the last entry's object ended past the allocation. Nothing in the truncated tail has a default initializer, so it corrupted nothing — but adding one would have made it a heap overrun. The reservation now ends at the last entry's full size. Measured on qwen3-14b decode, 7 binds, minimum over the run: bind-stage H2D 642,987 -> 65,441 ns (-90%), a single 180,480-byte arena copy in place of 46 copies totalling ~467 KB, PTO2 shared memory reserved 81,412,352 -> 77,440, graph heap high-water consumed 127,673,344 -> 106,399,744, and total device memory reserved 361,363,392 -> 270,857,280 (-86.3 MiB, -25%). The graph heap's *reservation* is untouched and still `PTO2_HEAP_SIZE`; sizing it from the measurement is separate work that depends on how heap positions are represented. cpput 113/113; a2a3 host_build_graph scene tests 34 passed + 1 skipped with every L3 resource child green; the a2a3sim and a5sim host_build_graph validation cases 4/4 each.
|
Warning Review limit reached
Next review available in: 52 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
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 21, 2026
ChaoWao
added a commit
that referenced
this pull request
Aug 21, 2026
…:set (#1951) `PTO2TaskSlotState::task` became a `SelfRelativePtr<PTO2TaskDescriptor>` in #1947, which deletes copy-assignment on purpose -- the stored value is a delta from the field's own address, so assigning one from a raw pointer or another field would silently retarget it. That change converted the three call sites in `test_hbg_graph_cache.cpp` it could see. The two in `LocalizesBoundaryScalarPoolWiderThanTaskPayload` and `RejectsBoundaryScalarPoolBeyondContract` arrived from #1941 while it was in review, so neither PR's own CI ever compiled both halves together, and `test_graph_cache` has not built on main since they merged: tests/ut/cpp/common/test_hbg_graph_cache.cpp:444:24: error: no match for 'operator=' (operand types are 'simpler::hbg::SelfRelativePtr<PTO2TaskDescriptor>' and 'PTO2TaskDescriptor*') Both now use `outer_slot.task.set(&outer_task)`, the form the file's other three slots already use. Verified locally: `test_graph_cache` and `test_a5_graph_cache` both build, and `ctest -R graph_cache -LE requires_hardware` passes 2/2.
poursoul
added a commit
to poursoul/simpler
that referenced
this pull request
Aug 21, 2026
`PTO2TaskSlotState::task` is a `SelfRelativePtr`, whose copy-assignment is deleted and which offers no `operator=(T *)`: a delta only means something relative to where it is stored, so every write goes through `set()`. `test_hbg_graph_cache.cpp` assigns a raw pointer at two of its five binding sites, which fails to compile: test_hbg_graph_cache.cpp:444: no match for 'operator=' (operand types are 'simpler::hbg::SelfRelativePtr<PTO2TaskDescriptor>' and 'PTO2TaskDescriptor*') Both sites are in the boundary-scalar-pool regressions added by hw-native-sys#1941, against the representation hw-native-sys#1947 introduced; the two landed without seeing each other, so ut-cpp is red on the default branch. Reproduced on a clean a5c6093. Two `std::lock_guard` uses become `std::scoped_lock` in the same file that the graph orchestration change touches: clang-tidy checks a modified file in full and `modernize-use-scoped-lock` rejects them. ctest: 114/114.
poursoul
added a commit
to poursoul/simpler
that referenced
this pull request
Aug 21, 2026
`PTO2TaskPayload` carried three fixed-capacity arrays sized by the argument caps — `fanin_local_ids[PTO2_MAX_FANIN]`, `scalars[MAX_SCALAR_ARGS]`, `tensors[MAX_TENSOR_ARGS]`, 4864 bytes. hw-native-sys#1947 made the tensor array shippable at less than its width by moving it last and striding the image by the bind's widest task. Two costs remained: a narrow task still paid the widest task's stride, and the fanin array did not participate at all. The arrays move into pool segments of the shared-memory image, and a payload names each region by a `SelfRelativePtr` — the same int32 delta representation hw-native-sys#1947 introduced for a slot's payload and descriptor. `sizeof(PTO2TaskPayload)` becomes a fixed 192 bytes, independent of all three caps, and the image carries what a bind actually used rather than what its widest task implies. Measured on DeepSeek-V4 flash decode (a2a3, 129 tasks / 86 GRAPH tasks), a per-task-cap layout would reserve 16,512 fanin ids, 4,128 tensors and 2,064 scalars; this bind uses 1,664 / 151 / 72. The tasks average 1.17 tensors, which is why striding by the widest leaves most of the reservation unused. What this retires: - `payload_stride` / `shipped_payload_stride`. A fixed-size payload makes the image's payload segment one `memcpy` instead of one per task. On dsv4 the restack goes from 133 `memcpy` calls to 8. - `node_stride` / `graph_node_stride`. `GraphNodeStorage` is type-strided again and `node_at()` is back to indexing. A Graph node's regions live in its execution storage, sized by the Definition's arg-table counts and indexed by each node's own `tensor_offset` / `scalar_offset`. - `runtime->sm_payload_stride` → `sm_image_bytes`. The device cannot recompute an image whose pool extents are the bind's cursors, and needs no pool base either: a payload resolves its own regions. H2D stays at one copy. The pools are segments of the image, so host-side `memcpy` fills them into the same upload buffer. Load-bearing details: - Deltas are re-taken in `compact_live_image`, which re-pitches the segments and therefore changes those distances, exactly as it already re-takes the slot bindings. Only the device copy is delta-invariant, because it moves a field and its target together. - The fanin cursor advances when the count is published, not at bind: `PTO2FaninBuilder` appends and dedups producers after the region is bound. The cursor equality asserted there doubles as a re-entrancy check on that window. - Every region is cache-line aligned so `init()`'s round-up scalar memcpy stays inside the task's own region and keeps its branch-free form. - Regions are exposed first-element-first, not per element: a store through an unrelated pointer in a loop body defeats hoisting of the delta load, and `build_payload`'s `args[]` writes are exactly that. - The pool bases are the orchestrator's own state, not ring-header fields. The device resolves a region through the payload's delta, so it never needs a base, and the header that crosses to it keeps its 192 bytes. - An outer GRAPH task binds only a fanin region. Its boundary arguments travel inside the submission image and `graph_reset_outer_payload` zeroes both argument counts, so it needs no tensor or scalar region at all. An unbound region survives the restack unbound, which every consumer's count guard assumes; `UnboundRegionsStayUnbound` holds that. - A bound region is inside its own mirror pool because the only binder is a bump cursor on that pool. `compact_live_image`'s translation depends on it, so it asserts it rather than re-deriving it, and `init` asserts that a non-zero count has a region to store through. - `init_per_ring` rejects a ring capacity whose mirror is past an int32 delta's reach, before it considers the region's own size: no region, however large, makes such a capacity usable. Image extents never exceed mirror extents, so that one check also bounds the shipped image; `attach_populated` re-tests it on the device side. `set()` leaves a field unbound rather than truncating, so an out-of-reach pool would otherwise read back as null and be dereferenced. - The payload is asserted trivially copyable and standard layout, per the wire struct rule. A deleted special member is still trivial, so `SelfRelativePtr` does not cost the payload either property — but without the assertion a future member could, and `compact_live_image` would keep `memcpy`ing it. `prefetch()` no longer touches byte 512, which was the early-dispatch block's old home and is past the end of a 192-byte payload; `init`'s comment naming that offset is corrected to the line the block now occupies. `GraphNodeStorage`'s two layout assertions now state the properties the argument pools need — that a node entry is at least `ChipTensor`-aligned, and that the tensor stride keeps the scalar pool aligned — rather than `sizeof % alignof`, which every type satisfies. `BindArenaH2d`'s attribute buffer grows to hold the three added pool counts, which 96 bytes would have truncated on a large bind. Testing: 8 variants build (a2a3/a5 x sim/onboard x hbg/tmrb), including ccec for the AICore gated path; C++ UT 114/114, with the compaction UT rewritten for pools (packed-pool shipping, per-region rebinding inside the image, unbound regions, layout delta reach).
ChaoWao
pushed a commit
that referenced
this pull request
Aug 21, 2026
…1952) `PTO2TaskPayload` carried three fixed-capacity arrays sized by the argument caps — `fanin_local_ids[PTO2_MAX_FANIN]`, `scalars[MAX_SCALAR_ARGS]`, `tensors[MAX_TENSOR_ARGS]`, 4864 bytes. #1947 made the tensor array shippable at less than its width by moving it last and striding the image by the bind's widest task. Two costs remained: a narrow task still paid the widest task's stride, and the fanin array did not participate at all. The arrays move into pool segments of the shared-memory image, and a payload names each region by a `SelfRelativePtr` — the same int32 delta representation #1947 introduced for a slot's payload and descriptor. `sizeof(PTO2TaskPayload)` becomes a fixed 192 bytes, independent of all three caps, and the image carries what a bind actually used rather than what its widest task implies. Measured on DeepSeek-V4 flash decode (a2a3, 129 tasks / 86 GRAPH tasks), a per-task-cap layout would reserve 16,512 fanin ids, 4,128 tensors and 2,064 scalars; this bind uses 1,664 / 151 / 72. The tasks average 1.17 tensors, which is why striding by the widest leaves most of the reservation unused. What this retires: - `payload_stride` / `shipped_payload_stride`. A fixed-size payload makes the image's payload segment one `memcpy` instead of one per task. On dsv4 the restack goes from 133 `memcpy` calls to 8. - `node_stride` / `graph_node_stride`. `GraphNodeStorage` is type-strided again and `node_at()` is back to indexing. A Graph node's regions live in its execution storage, sized by the Definition's arg-table counts and indexed by each node's own `tensor_offset` / `scalar_offset`. - `runtime->sm_payload_stride` → `sm_image_bytes`. The device cannot recompute an image whose pool extents are the bind's cursors, and needs no pool base either: a payload resolves its own regions. H2D stays at one copy. The pools are segments of the image, so host-side `memcpy` fills them into the same upload buffer. Load-bearing details: - Deltas are re-taken in `compact_live_image`, which re-pitches the segments and therefore changes those distances, exactly as it already re-takes the slot bindings. Only the device copy is delta-invariant, because it moves a field and its target together. - The fanin cursor advances when the count is published, not at bind: `PTO2FaninBuilder` appends and dedups producers after the region is bound. The cursor equality asserted there doubles as a re-entrancy check on that window. - Every region is cache-line aligned so `init()`'s round-up scalar memcpy stays inside the task's own region and keeps its branch-free form. - Regions are exposed first-element-first, not per element: a store through an unrelated pointer in a loop body defeats hoisting of the delta load, and `build_payload`'s `args[]` writes are exactly that. - The pool bases are the orchestrator's own state, not ring-header fields. The device resolves a region through the payload's delta, so it never needs a base, and the header that crosses to it keeps its 192 bytes. - An outer GRAPH task binds only a fanin region. Its boundary arguments travel inside the submission image and `graph_reset_outer_payload` zeroes both argument counts, so it needs no tensor or scalar region at all. An unbound region survives the restack unbound, which every consumer's count guard assumes; `UnboundRegionsStayUnbound` holds that. - A bound region is inside its own mirror pool because the only binder is a bump cursor on that pool. `compact_live_image`'s translation depends on it, so it asserts it rather than re-deriving it, and `init` asserts that a non-zero count has a region to store through. - `init_per_ring` rejects a ring capacity whose mirror is past an int32 delta's reach, before it considers the region's own size: no region, however large, makes such a capacity usable. Image extents never exceed mirror extents, so that one check also bounds the shipped image; `attach_populated` re-tests it on the device side. `set()` leaves a field unbound rather than truncating, so an out-of-reach pool would otherwise read back as null and be dereferenced. - The payload is asserted trivially copyable and standard layout, per the wire struct rule. A deleted special member is still trivial, so `SelfRelativePtr` does not cost the payload either property — but without the assertion a future member could, and `compact_live_image` would keep `memcpy`ing it. `prefetch()` no longer touches byte 512, which was the early-dispatch block's old home and is past the end of a 192-byte payload; `init`'s comment naming that offset is corrected to the line the block now occupies. `GraphNodeStorage`'s two layout assertions now state the properties the argument pools need — that a node entry is at least `ChipTensor`-aligned, and that the tensor stride keeps the scalar pool aligned — rather than `sizeof % alignof`, which every type satisfies. `BindArenaH2d`'s attribute buffer grows to hold the three added pool counts, which 96 bytes would have truncated on a large bind. Testing: 8 variants build (a2a3/a5 x sim/onboard x hbg/tmrb), including ccec for the AICore gated path; C++ UT 114/114, with the compaction UT rewritten for pools (packed-pool shipping, per-region rebinding inside the image, unbound regions, layout delta reach).
This was referenced Aug 22, 2026
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
The bind stage made 46 host-to-device transfers per bind and reserved 361 MB
of device memory, most of it for data the device never reads. Both come from the
same habit: every structure was shipped at the width of its type and copied on its
own.
One copy instead of forty-six. A slot's payload and descriptor are named by a
self-relative offset — an int32 delta from the field's own address — so the whole
ring image survives a single
memcpywith no pointer fix-up, which retires thehost-to-device relocation pass. The Graph submissions go up as one block rather
than one allocation and one copy each, and both the shared-memory image and that
submission block become tails of the runtime arena's own region, so they ride the
arena's copy. What is left is one arena copy plus one retained Definition object.
Only the bytes the device reads. The arena's host-only zone is dep-computation
scratch no device code touches, so it is no longer reserved on the device at all.
The shared-memory image ships pitched to the submitted task count rather than the
ring capacity, and the device-side reservation follows the same pitch — on qwen3-14b
decode, from 81,412,352 bytes to 77,440.
Strided by what a bind actually holds, not by the type.
PTO2TaskPayload'stensor array and
GraphNodeStorage's payload move last so each can be truncated,and the shipped arrays are then strided by the widest task and the widest node in
the bind.
sizeof(GraphNodeStorage)is not a power of two, so indexing alreadycompiled to a multiply; a runtime stride changes the operand from an immediate to a
register and adds no indirection on the dispatch path.
The int32 delta carries two bounds with it.
set()leaves the field unbound ratherthan storing a truncated delta, since unbound is the value every consumer already
tests for and a truncated one names unrelated memory;
attach_populatedrejects animage whose end exceeds the bound. On the host side
compact_live_imageasserts thesame two bounds, where exceeding them would read past the mirror's segments and ship
a corrupt image with nothing to say so.
Three defects fall out of the same reading, and none is an optimization — see
the commit message for each, including a Graph outer task's slot that was not reset
as it was claimed and was only accidentally correct (a fresh
new uint8_t[]comesback zero-filled, so a stale slot never appeared; a reused slot or mirror would have
read the previous run's state). Each is covered by a test that fails without the fix.
Relationship to #1932
This is the same change as #1932's original commit (
0f84de55), rebased ontocurrent
main— #1932's base is102df3d5, now five commits behind (missing#1936, #1937, #1875, #1940, #1942). The rebase changed nothing: both commits'
own diffstats are identical at 43 files, +1998/−635.
#1932 has since been rewritten into a different shape (
17115ed1, "Fix: pack HBGpayloads into the runtime arena", 54 files), so the two are no longer the same
change and one of them should be closed rather than both merged. Flagging that
rather than assuming which.
Measured effect
a2a3, dsv4 seven-Definition decode,
--rounds 5with the device run skipped,per-phase minima over the 8 warm passes, interleaved
A, B, A, Bwith both armsdriven by the same harness and the same environment stamp:
host_orchgraph_uploadrelocatesm_h2darena_h2dThe two repetitions' totals agree to 2 µs.
relocateandsm_h2dare retiredoutright, and
arena_h2dgrows because their work now rides the arena copy — 0.03 msin exchange for 0.105 ms.
host_orchagrees in sign but the magnitudes differ 6× andboth sit inside their own spread; I do not claim a real effect there, and this
change has no reason to touch orchestration.
Testing
ctest -LE requires_hardware— 114/114 (6 new cases from this change)both
🤖 Generated with Claude Code