Refactor: confine host_build_graph orchestration to the host - #2094
Open
poursoul wants to merge 1 commit into
Open
Refactor: confine host_build_graph orchestration to the host#2094poursoul wants to merge 1 commit into
poursoul wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (21)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change moves ChangesHost orchestration runtime split
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The refactor removes host orchestration from the device image and clears its callback pointers, but a host-only tensor_access address is still copied to the device. No current device use is identified, so the risk is bounded and mergeable with explicit owner awareness or follow-up to clear that field as well. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 28.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 17 files. (2 skipped: 2 unsupported.)
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 |
poursoul
force-pushed
the
refactor/confine-host-build-graph-orchestration-to
branch
from
September 2, 2026 04:10
9054d50 to
a881547
Compare
host_build_graph builds its graph on the host: the orchestration .so that drives it is dlopened by the host runtime, and the AICPU only ever runs schedulers. The sources implementing that orchestration nonetheless sat under shared/, so the aicpu target compiled all of them -- the ops table, the whole submit path, TensorMap, and Graph recording -- 1.6 MB of a 4.8 MB device image no device code can reach. Nothing under aicpu/, runtime/ or device/ names OrchestratorState or ChipTensorMap, and the AICPU .so gains no undefined symbol on any of the four variants once they are gone, which is what pins that claim. runtime_core.cpp, orchestrator.cpp and tensormap.cpp move under host/, which the build config already excludes from the aicpu target, so no build file changes. OrchestratorState::init goes with them: it calls ChipTensorMap::init_default, and left in shared/runtime_init.cpp it would have given the AICPU an unresolved reference -- which a shared library reports at load time rather than at link time. The AICPU no longer rebinds the ops table at boot, and the host clears rt->ops alongside rt->orchestrator before the copied zone is uploaded, so the device never receives a host address in that field. With one target compiling them, the weak markers those files carried select nothing; they only hid which definition wins behind link order, where nothing checks it, and a missing sink linked cleanly and silently did nothing. get_sys_cnt_aicpu and get_reg_ptr become ordinary definitions in host/aicpu_shims.cpp -- on the host neither is a fallback, since host orchestration needs a real monotonic clock and route_ready_once ODR-uses the doorbell inline -- and the clock's three copies collapse into one. The dep_gen and host_phase no-ops move to tests/ut/cpp/stubs/, carried by HBG_ORCH_SHARED_SOURCES, which the one test that wants the real capture does not use; that split is exclusive, so those need no weak marker either. RuntimeOps was written out three times, once per arch's orchestration_api.h and once in runtime_core.h, and the copies had to agree field for field since the .so calls through the table by offset. runtime_ops.h carries the definition and both sides include it.
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.
What
host_build_graphbuilds its graph on the host — the orchestration.sois dlopened by the host runtime, and the AICPU only ever runs schedulers. But the sources implementing that orchestration sat undershared/, so the aicpu target compiled all of them: the ops table, the whole submit path, TensorMap, and Graph recording.The AICPU device image drops from 4.8 MB to 3.1 MB — 1.6 MB (33%) of code no device code could reach.
Why it is safe
Nothing under
aicpu/,runtime/ordevice/namesOrchestratorStateorChipTensorMap, and thegraph_*free functions they define reach no device caller.The load-bearing check is on symbols, not on the build succeeding: a shared library links cleanly with unresolved symbols and only fails at
dlopen. So the undefined-symbol table of each AICPU.sowas captured before the move and compared after — zero new undefined symbols on all four variants (a2a3/a5 × sim/onboard).The three pieces
1. The ops table is host-only. The AICPU no longer rebinds it at boot, and the host now clears
rt->opsalongsidert->orchestratorbefore the copied zone is uploaded, so the device never receives a host address in that field.2.
runtime_core.cpp,orchestrator.cpp,tensormap.cppmove underhost/— which the build config already excludes from the aicpu target, so no build file changes.OrchestratorState::initgoes with them: it callsChipTensorMap::init_default, and left inshared/runtime_init.cppit would have handed the AICPU an unresolved reference.3. Weak markers become link seams. With one target compiling these files,
weakselects nothing — it only hid which definition wins behind link order, where nothing checks it, and a missing sink linked cleanly and silently did nothing.get_sys_cnt_aicpuandget_reg_ptrbecome ordinary definitions inhost/aicpu_shims.cpp(on the host neither is a fallback: host orchestration needs a real monotonic clock, androute_ready_onceODR-uses the doorbell inline), collapsing the clock's three copies into one. Thedep_gen/host_phaseno-ops move totests/ut/cpp/stubs/, carried byHBG_ORCH_SHARED_SOURCES— which the one test wanting the real capture does not use, so that split is exclusive and needs no weak marker either.Removing the weak markers immediately surfaced 6
undefined references at UT link time. Underweakthose same gaps would have been silent no-ops:host_phase_recordrecording nothing,dep_gencapturing nothing, tests still green.Also
RuntimeOpswas written out three times — once per arch'sorchestration_api.h, once inruntime_core.h— and the copies had to agree field for field, since the.socalls through the table by offset. A field added to one copy alone shifts every later entry and dispatches to the wrong function, with nothing to catch it at compile time.runtime_ops.hnow carries the one definition both sides include.Two comments in
graph_recorder_poolexplaining why that TU cannot includeruntime_core.hnamedRuntimeOpsas the type existing twice. The conflict isRuntimeContext— partial inorchestration_api.h, full inruntime_core.h— and it still stands; the comments now name it.What did NOT change
The ops table itself stays.
ChipWorkerdlopens each runtimeRTLD_LOCALso two runtimes' identically-named exports (simpler_init,simpler_run, …) cannot collide anddlclosecan actually unload one (#453). A runtime's symbols therefore never reach the global symbol table, and the orchestration.sohas no way to bind them by name. The table is not a legacy layer — it is how that boundary is crossed.Test
Build: 8 targets, zero warnings
Symbols: 0 new undefined on all four AICPU
.sos; both host shims confirmed absent from the host.so's dynamic symbol table (hiddenintact)ctest: 128/128Scene tests: a2a3sim 10 passed, a5sim 7 passed
pre-commit: clean
a2a3 onboard (real hardware): 35 passed / 1 skipped (host_build_graph) + 2 passed (tensormap_and_ringbuffer),
task-submitexit=0. No507018/507014/507899, and the run's own device log carries noFATAL, deadlock orHandleTaskTimeoutsignature.a5 hardware is covered by CI (
st-onboard-a5), not locally — this box is a2a3.