host_build_graph: route TensorCreateInfo initial values through the host view - #1905
host_build_graph: route TensorCreateInfo initial values through the host view#1905ChaoZheng109 wants to merge 1 commit into
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:
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 (27)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe host-build-graph runtime now initializes runtime-created GM-heap outputs through ChangesRuntime initial-value handling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR fixes host-side initial-value writes and adds a staged A5 path for devices without a host mapping. Physical A5 hardware has not exercised the chunked device-copy behavior, leaving a bounded compatibility risk; the PR is mergeable with explicit owner awareness and follow-up validation. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 |
3fab972 to
80e2880
Compare
review notes代码审查过的结论:修复定位准确, 下面是需要跟进的几点。 Should fix1. GM 堆的 host 映射是无条件的,代价随
建议改成首次 fill 时惰性映射:region 先登记为「尚未映射」, 2. 消息固定为
后三种发生时这条消息会把排查引向完全错误的方向; Consider3. 弱符号
4.
5.
描述里的「Not covered」一节可能已经过时PR 描述写「The a5 staged path has no silicon verification」。但新用例 我没能直接拉到那个 job 的日志确认用例确实执行、而非被 |
80e2880 to
a8bc4fd
Compare
|
Addressed 1, 2, 4, 5, 6; skipping 3. 1 — GM 堆映射改为惰性。 确认属实: 2 — fatal 消息区分失因。 新增 4 — 加了 RAII binding。 照同文件 5 — 直接传 6 — 覆盖度表述已更正,并做了你没能完成的那步确认。 3 — 不做。 哨兵自检防的是链接顺序/构建配置出岔子这种假想场景,需要动强符号导出与 验证:C++ UT 107/107;a2a3sim 10 passed / a5sim 6 passed;a2a3 上板 |
a8bc4fd to
fbb6da9
Compare
…ost view set_initial_value segfaulted the orchestrator on every host_build_graph path. fill_tensor_initial_value memcpy'd to ChipTensor::buffer.addr, and on this runtime that is a GM-heap device address the orchestrating host cannot load. The file is a verbatim copy of the tensormap_and_ringbuffer one, where the orchestrator runs on the AICPU and the store is correct; the copy kept the store and lost its premise. HostTensorAccessor is the seam host_build_graph added for exactly that difference, and get_tensor_data / set_tensor_data were routed through it, but the fill was not. Nothing caught it because host_build_graph had no set_initial_value user at all: all three callers in the tree are tensormap_and_ringbuffer cases, and there was no host_build_graph scalar_data case. Two things were missing, and only both together make it work. The fill now goes through the accessor, from the orchestrator call sites that hold one and can report a failure; init_tensor_from_create_info goes back to materializing metadata alone. And the GM heap is registered as a region, since the only add() call site is the per-staged-tensor loop, so the heap resolved to nothing. host_tensor_fill's weak fallback sits in pto_orchestrator.cpp rather than beside the host_tensor_read / host_tensor_write ones in pto_runtime2.cpp: its only caller is there, and the C++ unit tests that link the orchestrator do not link the runtime translation unit, so the pair's placement does not carry over. Regions are kind-scoped rather than pooled. read/write serve the scalar-access API and see staged tensors only, which is what keeps a runtime-created output unreadable during graph construction; that rule is enforced today by the heap's absence from the table, and pooling would have turned a documented failure into a silent read of an unwritten buffer. fill sees the heap only, matching the one kind of buffer a TensorCreateInfo can produce. The heap registers with no host view of its own, and no mapping either. Both would be charged to every bind for a facility most orchestrations never use, so the halHostRegister over the whole heap — 256 MB by default, 2 GiB on the dsv4 case — is deferred to the first fill that needs it and made once. A run that sets no initial value pays nothing. Where a mapping cannot be had at all (a5 onboard, whose DeviceRunnerBase default returns nullptr), fill stages the bytes instead of failing. A fallback view in the shape add() uses would have to span the heap, because find_region indexes host_view + offset; that size is forced by read, which serves arbitrary offsets at arbitrary times and needs the bytes to persist. fill is neither: it is write-only and its source is one value repeated, so one period of the pattern in a bounded buffer, pushed in chunks with copy_to_device, covers any span — the same mechanism set_tensor_data has used on a5 since paged_attention. Host memory is 64 KiB for the run rather than proportional to the heap, and the pushed bytes are the filled buffer's. The push is per-call, never deferred to a flush. The heap is reclaimed and re-let within one orchestration, so two fills can name the same address and only the order they were issued in is correct. A fill that does not happen reports which of the three things stopped it — element width, span outside the heap, failed device copy — rather than naming the one that motivated the diagnostic. A region without a host view is not one of them: on a platform with no host-map path that is the ordinary state, and the staged push that follows reports its own outcome. The accessor's tensor_access pointers are installed by an RAII binding in the shape of the GraphHostStateBinding beside it, so they do not outlive the stack accessor they name; run_host_orchestration takes the heap span its caller already summed under overflow check rather than re-deriving it. An initial value inside a Graph body is marked unsupported. Recording addresses an internal node's outputs in the private range based at GRAPH_RECORD_VIRTUAL_BASE, and every submission of the resulting Definition materializes its own from a heap block whose prior contents it never reads, so a value written during recording reaches none of the replays. Since hw-native-sys#1897 that cannot be answered by re-running the body — its outer shell is published before the body is recorded — so graph_commit reports PTO2_ERROR_INVALID_ARGS and the run fails where it would otherwise have replayed outputs the orchestration believes it initialized. SCALAR_DATA_ACCESS.md states the restriction and the two ways around it. tests/st/{a2a3,a5}/host_build_graph/initial_value/ covers both materialization paths — alloc_tensors and a submitted task's output, via a dummy task that dispatches no kernel so the value survives to its consumer. Each value is observed by a kernel rather than get_tensor_data, which cannot read a runtime-created tensor here. Verified on a2a3 hardware, including one run with ring_heap raised to the dsv4 case's 2 GiB to confirm halHostRegister takes a span that size (3.4 s for the case, registration included). The case is level 2 and not manual, so st-onboard-a5 — pytest examples tests/st --platform a5 --exclude-level 4 — runs it on a5 silicon, which is where the staged path executes. It is additionally exercised on a2a3 by stubbing the sim runner's register_device_memory_to_host to return nullptr, so both branches of the mapping decision are covered on this box: the initial_value case and paged_attention (the existing get_tensor_data user, which then runs mirrored) pass on a2a3sim and a5sim under the stub. The mapped path passes the a2a3sim and a5sim host_build_graph suites.
与 #1922 的关系:这两个 PR 是互斥方案,不是先后关系前一轮的 5 条意见都已处理(惰性映射、状态枚举、RAII binding、
两者文件上不冲突(#1905 动 1. #1922 合入后,真实用户归零
而 2. Graph 变体已经在 main 上,不是未来的假设
也就是说 即便合了 #1905,dsv4 三份编排里的 Graph 那份照样用不了这个接口,还是得走 device-side。本 PR 修好的能力,对它要服务的用例有三分之一从一开始就不适用。而这不是实现没做到 —— 初值与 record/replay 是语义上的冲突,补不上。 3. 那 6 行不是 codegen 契约它们随 #1838 一次性进入 建议#1922 先合;本 PR 关掉;另开一个删除 PR。 本 PR 里唯一与接口存废无关、值得单独捞出来的是两处文档修正:
删除 PR 的范围:
删除时真正要小心的地方不是 API 表面,是布局// --- Bytes [0, 32): TensorCreateInfo-only fields ---
// These occupy the same positions as ChipTensor::buffer, ChipTensor::owner_task_id,
// and ChipTensor::start_offset. ...
uint64_t initial_value; // [0, 8)
bool has_initial_value; // [8, 9)
uint8_t __pad1__[7]; // [9, 16)这前 16 字节是刻意与 要么换成显式 padding 保持布局不变(省事,但留一个无名洞),要么连同那个 memcpy 技巧一起重新设计。这是删除 PR 的实际风险所在。 需要确认的一件事按 如果确认还有别的模型在用,那就退回次选方案:#1922 先合,本 PR 作为过渡实现后合,同时记一条 issue 指向删除,并补上一条 debug 级提示 —— |
|
关闭。#1922 已合入 main( 关闭理由
为零真实用户的接口背 ~330 行运行时代码不划算。 更正:上一条评论建议捞出的两处文档修正,其实不需要捞那两处的措辞问题都是本 PR 自己造成的,不是 main 上独立存在的缺陷:
关掉本 PR,两处随之消失。所以没有可捞的文档修正。 顺带确认那条论断确实是错的: 一个确实独立存在、仍在 main 上的缺陷
后续删除 |
1 participant
What this fixes
TensorCreateInfo::set_initial_valuesegfaulted the orchestrator on everyhost_build_graphpath.fill_tensor_initial_valuememcpy'd straight toChipTensor::buffer.addr— a GM-heap device address that the orchestratinghost cannot load.
The file is a verbatim copy of the
tensormap_and_ringbufferone, where theorchestrator runs on the AICPU and that store is correct. The copy kept the
store and lost its premise.
HostTensorAccessoris the seamhost_build_graphadded for exactly this difference —
get_tensor_data/set_tensor_datawererouted through it, the fill was not.
It went unnoticed because
host_build_graphhad noset_initial_valueuserat all: all three callers in the tree are
tensormap_and_ringbuffercases,and there was no
host_build_graphscalar_datacase. Moving the DeepSeek-V4case onto this runtime is what first steps on it.
What changed
Two things were missing; only both together make it work.
hold one and can report a failure.
init_tensor_from_create_infogoes backto materializing metadata alone.
add()call site is theper-staged-tensor loop, so the heap resolved to nothing.
Regions are kind-scoped, not pooled.
read/writesee staged tensors only,which is what keeps a runtime-created output unreadable during graph
construction. That documented rule is enforced today by the heap's absence
from the table; pooling would have turned a clean failure into a silent read of
an unwritten buffer.
fillsees the heap only.The heap registers with no host view — and no mapping either. Both would be
charged to every bind for a facility most orchestrations never use, so the
halHostRegisterover the whole heap (256 MB by default, 2 GiB on the dsv4case) is deferred to the first
fillthat needs it and made once. A run thatsets no initial value pays nothing.
Where a mapping cannot be had at all,
fillstages instead of failing. Afallback view in
add()'s shape would have to span the heap, becausefind_regionindexeshost_view + offset— that size is forced byread,which serves arbitrary offsets at arbitrary times.
fillis neither: write-only,source is one value repeated, so one period of the pattern in a bounded buffer
pushed in chunks with
copy_to_devicecovers any span — the same mechanismset_tensor_datahas used on a5 onboard sincepaged_attention. Hostmemory is 64 KiB for the run, and the pushed bytes are the filled buffer's.
A fill that does not happen says which of four things stopped it — element
width, span outside the heap, absent host view, failed device copy — rather than
naming the one that motivated the diagnostic.
The push is per-call, never deferred: the heap is reclaimed and re-let within
one orchestration, so two fills can name the same address and only their issue
order is correct.
halHostRegister), store lands directly — no copycopy_to_deviceAn initial value inside a Graph body is unsupported
Recording addresses an internal node's outputs in the private range based at
GRAPH_RECORD_VIRTUAL_BASE, and every submission of the resulting Definitionmaterializes its own from a heap block whose prior contents it never reads — so
a value written during recording reaches none of the replays.
Since #1897 that cannot be answered by re-running the body on the ordinary path:
the outer shell is published before the body is recorded. The recording is
marked unsupported and
graph_commitreportsPTO2_ERROR_INVALID_ARGS, so therun fails loudly where it would otherwise have replayed outputs the
orchestration believes it initialized.
SCALAR_DATA_ACCESS.mdstates therestriction and the two ways around it (allocate outside the body and pass it
across the boundary, or have a task write the padding).
Verification
initial_valuecase passes, plus one run withring_heapraised to the dsv4 case's 2 GiB to confirmhalHostRegistertakes a span that size (3.4 s for the case, registration included).
level=2and not manual, sost-onboard-a5(
pytest examples tests/st --platform a5 --exclude-level 4) runs it, which iswhere the staged path executes. That job is green.
register_device_memory_to_hostto returnnullptr, so both branches of themapping decision are covered locally: the
initial_valuecase andpaged_attention(the existingget_tensor_datauser, mirrored under thestub) pass on a2a3sim and a5sim. Stub reverted.
host_build_graphsuites pass.FATAL(code=5)rather than producing wrong numbers, as documented.New cases in
tests/st/{a2a3,a5}/host_build_graph/initial_value/cover bothmaterialization paths (
alloc_tensors, and a submitted task's output via adummy task that dispatches no kernel so the value survives to its consumer).
Each value is observed by a kernel rather than
get_tensor_data, which cannotread a runtime-created tensor here.
Not covered
Nothing outstanding. An earlier revision of this description claimed the a5
staged path had no silicon verification;
st-onboard-a5does run the new case,so that caveat is withdrawn.