Refactor: stop host_build_graph's host side reaching into device facilities - #2098
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughHost orchestration profiling now measures phase durations with ChangesHost orchestration profiling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR switches host orchestration profiling to monotonic nanosecond timing and removes unused host shims without changing normal-build behavior or device-side execution. No actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
poursoul
changed the title
ea7d5ec to
c192f24
Compare
…lities hw-native-sys#2094 moved host_build_graph's orchestration to the host. Two device-side dependencies survived that move, both of them reaching for something the host cannot actually use. The orchestrator's per-sub-step profiling called get_sys_cnt_aicpu, a device cycle counter. On the host that resolved to a stand-in reading CLOCK_MONOTONIC and scaling it by PLATFORM_PROF_SYS_CNT_FREQ, so the orchestrator turned nanoseconds into notional device cycles, accumulated those, and the bind logged them as `cycles=` -- neither a real cycle count nor a readable duration, and a dependency on a device frequency constant for timing that never leaves the host. The accumulators now read clock_gettime through a file-local inline and carry nanoseconds: OrchProfilingData's five *_cycle fields and fanin_wait_cycle become *_ns, the macros are ORCH_STEP_START/LAP, and the log line reads `ns=`. That was the host's only use of get_sys_cnt_aicpu, so host/aicpu_shims.cpp goes away. Its other definition, get_reg_ptr, had no caller at all: disassembling the host .so under the default, ORCH_PROFILING and SCHED_PROFILING configurations finds zero call sites in every one, and the comment explaining it named a route_ready_once path that exists nowhere in the tree. OrchestratorState carried SchedulerState *scheduler, pointing at a struct that lives in device memory the host never maps -- residue from when both ran on the AICPU in one address space and the orchestrator could enqueue into the scheduler's queues directly. Its own comment said "For simulated mode only", and nothing read it: submit_task copied it into a local whose only use was a (void) cast, and two call sites asserted it non-null without dereferencing. The member, the init() parameter that filled it, and both assertions are gone. That was orchestrator.h's only use of SchedulerState, so the header no longer includes scheduler/scheduler.h -- 1336 lines of device dispatch logic the host compiled to reach a pointer it never followed. Eight unit tests that had been reaching SchedulerState and SchedulerLayout through that chain now include it directly. The AICPU side is untouched: libaicpu_kernel.so carries its own get_sys_cnt_aicpu from platform/.../device_time.cpp -- 195 call sites under SCHED_PROFILING, zero undefined references -- and never compiled the deleted file. Built in all four profiling configurations, since a path that exists only under a conditional is invisible to a single build.
c192f24 to
d4fb085
Compare
2 participants
Follow-up to #2094, which moved host_build_graph's orchestration to the host. That move left the host side still reaching for device-side facilities in two places.
1. The clock went through a device counter to read a host wall-clock
orchestrator.cppruns on the host, but its per-sub-step profiling calledget_sys_cnt_aicpu— a device cycle counter. On the host that resolved to a stand-in readingCLOCK_MONOTONICand scaling it byPLATFORM_PROF_SYS_CNT_FREQ:Neither a real cycle count nor a readable duration, and it made host timing depend on a device frequency constant. It now reads
clock_gettimedirectly and carries nanoseconds —OrchProfilingData's*_cyclefields become*_ns, the macros areORCH_STEP_START/LAP, and the bind's log line readsns=.This changes a DFX log line's unit and field names.
SIMPLER_ORCH_PROFILINGis off by default, so nothing on a normal build moves.That was the host's only caller of that symbol, so
host/aicpu_shims.cppis deleted. Its other definition,get_reg_ptr, had no caller at all — disassembling the host.sounder default,ORCH_PROFILINGandSCHED_PROFILINGfinds zero call sites in every one. The comment justifying it named aroute_ready_oncepath that exists nowhere in the tree.2. The orchestrator held a pointer into the scheduler
OrchestratorStatecarriedSchedulerState *scheduler— pointing at a struct that lives in device memory the host never maps. It is tmr-era residue from when both ran on the AICPU in one address space and the orchestrator could enqueue into the scheduler's queues directly; its own comment said "For simulated mode only".Nothing read it.
submit_taskcopied it into a local whose only use was a(void)cast, and two call sites asserted it non-null without ever dereferencing. The member, theinit()parameter that filled it, and both assertions are gone.That was
orchestrator.h's only use ofSchedulerState, so the header stops includingscheduler/scheduler.h— 1336 lines of device dispatch logic the host compiled to reach a pointer it never followed. Eight unit tests that had been reachingSchedulerState/SchedulerLayoutthrough that chain now include it directly.The AICPU side is untouched
libaicpu_kernel.socarries its ownget_sys_cnt_aicpufromplatform/.../device_time.cpp— 195 call sites underSCHED_PROFILING, zero undefined references — and never compiled the deleted file. The two.sos are each loadedRTLD_LOCAL, so their symbols were never shared.Test
SIMPLER_ORCH_PROFILING,SIMPLER_SCHED_PROFILING,SIMPLER_DFX=0— zero warnings. A path that only exists under a conditional is invisible to a single build..sohas zero call sites and zero undefined references for both removed symbols under every configuration;libaicpu_kernel.sounchanged.ctest: 128/128Not run on real hardware — this touches a DFX path that is off by default. CI's onboard jobs cover it.
An unrelated include-hygiene pass that had been sharing this branch is split out into its own PR, since it is a mechanical 48-file change with a different subject.