Fix: gate the device-phase timing buffer's per-run transfers on capture - #1657
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:
📝 WalkthroughWalkthroughChangesDevice phase capture
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceRunnerBase
participant DeviceWallBuffer
participant CApiShared
DeviceRunnerBase->>DeviceWallBuffer: reset and read timing data when capture is enabled
DeviceRunnerBase->>DeviceWallBuffer: skip transfers when capture is disabled
CApiShared->>DeviceRunnerBase: query device_phase_capture_enabled()
DeviceRunnerBase-->>CApiShared: capture state
CApiShared->>CApiShared: emit markers when enabled
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
6811a1d to
bc5d045
Compare
Fixes hw-native-sys#1642 Every simpler_run reset the onboard device-phase buffer over H2D and read the phase region and task-timing tail back over D2H. The readback exists only for device-domain [STRACE] markers, so runs that could not emit those markers still paid three marker-only transfers. Use one capture predicate for buffer preparation, readback, and marker emission. It requires SIMPLER_HOST_STRACE, an enabled SIMPLER_DEVICE_STRACE_ENABLE setting, and a log threshold that admits LOG_TIMING. Disabled capture publishes a null device buffer base, so AICPU stamping is also skipped; re-enabling capture restores the existing allocation. Add no-hardware coverage for compile-time, environment, and log-level gates, and align the DFX documentation with the onboard behavior. The sim path retains its emission-only environment gate. The task-timing tail still transfers when capture is enabled but no task is tagged; that is observation 2 and remains follow-up work. Co-authored-by: ChaoZheng109 <zhengchao47@huawei.com>
bc5d045 to
a2ef4cf
Compare
Fixes #1642.
Problem
Every
simpler_runperformed three device↔host transfers around the AICPU device-phase / task-timing buffer, all unconditional:ensure_device_wall_buffer()H2D resetread_device_wall_ns()phase-region D2Hread_device_wall_ns()task-timing tail D2HThe only consumer of the readback is
emit_device_phase_markers(), which is double-gated (compile-timeSIMPLER_HOST_STRACE—STRACE_DEV_SPAN_ATcompiles to nothing when off — and runtimeSIMPLER_DEVICE_STRACE_ENABLE). The producers were not gated: the only guard was "is the buffer allocated", which the lazy allocator makes true for the process lifetime after the first run. No other code readslast_device_wall_ns/last_device_phase_ns/last_task_slot_*. So with device strace disabled, all three transfers ran every run and the data was discarded — per-run latency on the dispatch/decode hot path.Fix
Gate the producer on the same condition as the consumer.
device_phase_capture_enabled()(compile gate ∧ runtime env) is now the single predicate both sides share:ensure_device_wall_buffer()early-returns when capture is off → no allocation, no H2D reset (transfer Multi-threaded AICPU Scheduler with Parallel Task Dispatch #1). The buffer stays unallocated, soKernelArgs::device_wall_data_basestays0and the AICPU stamps nothing —device_phase_aicpu.hno-ops at base 0, so no device-side write is stranded and the chip cannot hang.read_device_wall_ns()early-returns when capture is off → no D2H readbacks (transfers Refactor AICPU core assignment to support dynamic block distribution #2, efactor: Rename Graph to Runtime to Better Reflect Its Responsibility #3).emit_device_phase_markers()now calls the shared predicate instead of its own localdevice_profiling_enabled()env read (single source of truth; also drops the now-unused<cstdlib>include).When capture is on, the code path is byte-identical to before (allocate + reset + read + emit).
Scope
Verification
a2a3anda5onboard runtimes (host_build_graph + tensormap_and_ringbuffer) — clean. Pre-commit clang-format / clang-tidy / cpplint pass.npu-smireturns no chip), so the device-behavior check (markers still emitted with capture on; absent with capture off; no regression) is left to CI'sst-onboard-a2a3/st-onboard-a5jobs. The change is correct by construction: with capture on the path is unchanged, and the AICPU null-base no-op is an existing, documented guarantee.