Refactor: move per-run simpler_run state to its correct lifecycle - #1242
Merged
ChaoWao merged 1 commit intoJul 2, 2026
Merged
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a ChangesCallConfig-Driven Run/Bind Refactor and Runtime Layout
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
ChaoWao
force-pushed
the
worktree-serene-napping-unicorn
branch
from
July 2, 2026 00:50
007a452 to
2828962
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/a5/platform/onboard/host/device_runner.h (1)
86-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale Doxygen params on
run().The
@param block_dim/@param launch_aicpu_numentries no longer match theconst CallConfig &configsignature, andconfigis undocumented.📝 Proposed doc fix
- * `@param` runtime Runtime to execute (will be modified to - * initialize workers) - * `@param` block_dim Number of blocks (1 block = 1 AIC + 2 AIV) - * `@param` launch_aicpu_num Number of AICPU instances (default: 1) + * `@param` runtime Runtime to execute (will be modified to initialize workers) + * `@param` config Per-run CallConfig: block_dim (0 = auto), aicpu_thread_num, + * diagnostic enables + output_prefix (latched via + * apply_call_config), and ring sizing overrides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/a5/platform/onboard/host/device_runner.h` around lines 86 - 108, Update the Doxygen for DeviceRunner::run so it matches the current signature: remove the stale `@param` entries for block_dim and launch_aicpu_num, and document the actual const CallConfig &config parameter instead. Keep the existing run() behavior description intact and make sure the parameter list reflects the symbols in DeviceRunner::run and CallConfig.src/a2a3/platform/onboard/host/device_runner.h (1)
93-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale Doxygen params on
run().The
@param block_dim/@param launch_aicpu_numentries no longer match the newconst CallConfig &configsignature, andconfigis undocumented.📝 Proposed doc fix
- * `@param` runtime Runtime to execute (will be modified to - * initialize workers) - * `@param` block_dim Number of blocks (1 block = 1 AIC + 2 AIV) - * `@param` launch_aicpu_num Number of AICPU instances (default: 1) + * `@param` runtime Runtime to execute (will be modified to initialize workers) + * `@param` config Per-run CallConfig: block_dim (0 = auto), aicpu_thread_num, + * diagnostic enables + output_prefix (latched via + * apply_call_config), and ring sizing overrides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/a2a3/platform/onboard/host/device_runner.h` around lines 93 - 115, The Doxygen for DeviceRunner::run is stale because it still documents block_dim and launch_aicpu_num even though the signature now takes const CallConfig &config. Update the comment to describe config instead, document its fields or intent, and remove the obsolete parameter entries so the docs match run(Runtime &runtime, const CallConfig &config) and the implementation context in DeviceRunner.
🧹 Nitpick comments (2)
src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp (1)
1152-1157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
runtime_device_copy_size()for the invalidate range.This path should stay byte-for-byte aligned with the upload/allocation sizing contract; duplicating the
offsetof + task_count * sizeof(Task)formula makes future layout changes easy to miss.Proposed refactor
-#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Winvalid-offsetof" - const size_t runtime_prefix_bytes = - offsetof(Runtime, tasks) + static_cast<size_t>(runtime->get_task_count()) * sizeof(Task); -#pragma GCC diagnostic pop + const size_t runtime_prefix_bytes = runtime_device_copy_size(*runtime); cache_invalidate_range(runtime, runtime_prefix_bytes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp` around lines 1152 - 1157, The invalidate-range calculation is duplicating the runtime upload/allocation sizing contract, so update the AICPU executor path to reuse runtime_device_copy_size() instead of recomputing offsetof(Runtime, tasks) plus task_count * sizeof(Task). Make the cache_invalidate_range call consume that shared size value directly so the invalidation stays byte-for-byte aligned with the existing contract and future Runtime layout changes only need to be fixed in one place.src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp (1)
1157-1162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
runtime_device_copy_size()for the invalidate range.This path should stay byte-for-byte aligned with the upload/allocation sizing contract; duplicating the
offsetof + task_count * sizeof(Task)formula makes future layout changes easy to miss.Proposed refactor
-#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Winvalid-offsetof" - const size_t runtime_prefix_bytes = - offsetof(Runtime, tasks) + static_cast<size_t>(runtime->get_task_count()) * sizeof(Task); -#pragma GCC diagnostic pop + const size_t runtime_prefix_bytes = runtime_device_copy_size(*runtime); cache_invalidate_range(runtime, runtime_prefix_bytes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp` around lines 1157 - 1162, The invalidate-range size in the runtime upload path is duplicating the allocation sizing formula, so update the `aicpu_executor.cpp` logic to call `runtime_device_copy_size()` instead of recomputing `offsetof(Runtime, tasks) + task_count * sizeof(Task)`. Keep `cache_invalidate_range(runtime, ...)` using that shared helper so the `Runtime`/`Task` layout contract stays centralized and in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/a2a3/platform/onboard/host/device_runner.h`:
- Around line 93-115: The Doxygen for DeviceRunner::run is stale because it
still documents block_dim and launch_aicpu_num even though the signature now
takes const CallConfig &config. Update the comment to describe config instead,
document its fields or intent, and remove the obsolete parameter entries so the
docs match run(Runtime &runtime, const CallConfig &config) and the
implementation context in DeviceRunner.
In `@src/a5/platform/onboard/host/device_runner.h`:
- Around line 86-108: Update the Doxygen for DeviceRunner::run so it matches the
current signature: remove the stale `@param` entries for block_dim and
launch_aicpu_num, and document the actual const CallConfig &config parameter
instead. Keep the existing run() behavior description intact and make sure the
parameter list reflects the symbols in DeviceRunner::run and CallConfig.
---
Nitpick comments:
In `@src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp`:
- Around line 1157-1162: The invalidate-range size in the runtime upload path is
duplicating the allocation sizing formula, so update the `aicpu_executor.cpp`
logic to call `runtime_device_copy_size()` instead of recomputing
`offsetof(Runtime, tasks) + task_count * sizeof(Task)`. Keep
`cache_invalidate_range(runtime, ...)` using that shared helper so the
`Runtime`/`Task` layout contract stays centralized and in sync.
In `@src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp`:
- Around line 1152-1157: The invalidate-range calculation is duplicating the
runtime upload/allocation sizing contract, so update the AICPU executor path to
reuse runtime_device_copy_size() instead of recomputing offsetof(Runtime, tasks)
plus task_count * sizeof(Task). Make the cache_invalidate_range call consume
that shared size value directly so the invalidation stays byte-for-byte aligned
with the existing contract and future Runtime layout changes only need to be
fixed in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9cca73d-16d6-4c5b-8d54-d5563116def3
📒 Files selected for processing (32)
docs/chip-level-arch.mddocs/dynamic-linking.mdsrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/platform/onboard/host/device_runner.hsrc/a2a3/platform/sim/host/device_runner.cppsrc/a2a3/platform/sim/host/device_runner.hsrc/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a2a3/runtime/host_build_graph/host/runtime_maker.cppsrc/a2a3/runtime/host_build_graph/runtime/runtime.cppsrc/a2a3/runtime/host_build_graph/runtime/runtime.hsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/platform/onboard/host/device_runner.hsrc/a5/platform/sim/host/device_runner.cppsrc/a5/platform/sim/host/device_runner.hsrc/a5/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a5/runtime/host_build_graph/host/runtime_maker.cppsrc/a5/runtime/host_build_graph/runtime/runtime.cppsrc/a5/runtime/host_build_graph/runtime/runtime.hsrc/common/platform/include/common/host_api.hsrc/common/platform/onboard/host/c_api_shared.cppsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/sim/host/c_api_shared.cppsrc/common/platform/sim/host/device_runner_base.cppsrc/common/platform/sim/host/device_runner_base.hsrc/common/task_interface/prepare_callable_common.hsrc/common/worker/chip_worker.cppsrc/common/worker/chip_worker.hsrc/common/worker/pto_runtime_c_api.htests/ut/cpp/common/test_l3_l2_orch_comm_sim_runner.cpptests/ut/cpp/hardware/test_l3_l2_orch_comm_onboard_runner.cpptests/ut/cpp/stubs/test_stubs.cpp
💤 Files with no reviewable changes (1)
- src/common/task_interface/prepare_callable_common.h
Four related cleanups that stop simpler_run from doing per-run work that belongs to longer-lived scopes, and shrink the hbg Runtime upload: - HostApi is built once at load time as a file-scope `static const` table instead of reassembling its 12 context-free function pointers on every simpler_run (each wrapper recovers its runner from a thread-local, so one table is valid for every runner and run). Completes hw-native-sys#1227. - host_build_graph Runtime is reordered so all device-read fields form a contiguous prefix ending in tasks[], with a host-only tail after it. runtime_device_copy_size() now uploads only offsetof(tasks) + get_task_count()*sizeof(Task), truncating the unpopulated slots of the fixed 131072-entry tasks[] array; the AICPU cache-invalidate matches. offsetof static_asserts guard the layout. Safe because the device never reads tasks[i] for i >= next_task_id. - The two-step callable bind (bind_callable_to_runtime returning a BindCallableResult, then bind_callable_to_runtime_impl) is merged into one runner facade so the CallableState-owned signature pointer no longer crosses the c_api boundary; BindCallableResult is removed. - CallConfig is threaded through simpler_run as a single pointer instead of 14 unpacked C-ABI params plus six runner->set_*_enabled() calls. run() takes const CallConfig& and latches the diagnostic enables via apply_call_config() at entry. Verified on a2a3 silicon (host_build_graph + tensormap_and_ringbuffer) and in simulation on a2a3 + a5.
ChaoWao
force-pushed
the
worktree-serene-napping-unicorn
branch
from
July 2, 2026 01:58
2828962 to
9052868
Compare
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
Four related cleanups that stop
simpler_runfrom doing per-run work that belongs to longer-lived scopes, plus a shrink of the host_build_graphRuntimeupload. Each is independent; grouped here as one reviewable change.HostApi built once at load time. The 12-pointer
HostApitable was reassembled on everysimpler_run(onboard + sim). Its wrappers are context-free — each recovers its runner from a thread-local — so a singlestatic consttable is valid for every runner and run. Completes the Refactor: move HostApi out of Runtime into a shared explicit parameter #1227 direction (which moved HostApi offRuntimebut left it a per-run local).hbg Runtime upload shrunk to the populated task prefix.
host_build_graphuploaded the whole ~92 MBRuntimeimage every run, dominated by the fixedtasks[131072]array. Members are reordered so all device-read fields form a contiguous prefix ending intasks[], with a host-only tail after it.runtime_device_copy_size()now returnsoffsetof(tasks) + get_task_count()*sizeof(Task), uploading only populated slots; the AICPU cache-invalidate matches.offsetofstatic_asserts guard the layout. Safe because the device never readstasks[i]fori >= next_task_id(get_task()bounds-checks). The trbdevsub-struct pattern is inapplicable here because hbg embedsstd::atomic<int> faninin the uploaded image, so this is an explicit prefix boundary rather than a nested descriptor.Two-step callable bind merged into one facade.
bind_callable_to_runtimereturned aBindCallableResultcarrying a raw pointer intoCallableState's cached signature, which the c_api then handed tobind_callable_to_runtime_impl. Both halves are folded into one runner facade so that pointer no longer crosses the c_api boundary;BindCallableResultis removed.CallConfig threaded through simpler_run. The C ABI unpacked
CallConfiginto 14 positional params, and the c_api re-scattered them across sixrunner->set_*_enabled()calls.simpler_runnow takesconst CallConfig*;run()takesconst CallConfig&and latches the diagnostic enables viaapply_call_config()at entry. Theenable_*_members are kept (≈50 downstream readers in the collector paths).Testing
host_build_graphon a2a3sim (10 passed) and a5sim (7 passed)host_build_graph(10 passed, 1 skipped) andtensormap_and_ringbuffer(33 passed, 1 skipped)Note: onboard was validated per-refactor before a rebase onto #1234; after rebasing, both sim suites were re-run green.
tests/ut/cpphas a pre-existing gtest link-ABI failure in this environment (affects untouched binaries too) and was not run.