[WIP] Update: rename TensorDump internals to ArgsDump#1270
Open
indigo1973 wants to merge 1 commit into
Open
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:
📝 WalkthroughWalkthroughThis PR renames the "tensor dump" subsystem to "args dump" throughout the codebase, updating types ( ChangesTensorDump to ArgsDump Rename
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs a comprehensive renaming of the "TensorDump" diagnostic sub-feature to "ArgsDump" across the codebase, including updates to documentation, Python tools, C++ runtime headers, and platform implementation files. The review feedback points out a couple of missed spots where "tensor" terminology or prefixes were still being used: specifically, an exception message in l0_swimlane.py and several helper functions in args_dump_collector.cpp that should be renamed to maintain consistency with the rest of the changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp (1)
114-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Dump tensor" comment left behind.
Line 115's comment was updated to reference args dump, but the section header on Line 114 still reads "Dump tensor state". Should be renamed for consistency with the rest of this rename PR.
✏️ Proposed fix
- // ===== Dump tensor state ===== + // ===== Args dump state ===== Runtime *runtime_{nullptr}; // Cached for args dump access in try_dispatch_task🤖 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 114 - 115, The section header comment in aicpu_executor.cpp is stale and still says “Dump tensor state” even though the nearby Runtime cache comment and try_dispatch_task usage now refer to args dump access. Update the header comment near runtime_ in aicpu_executor.cpp so it matches the new args-dump naming used in this rename PR and stays consistent with the surrounding code.src/a5/platform/onboard/host/device_runner.h (1)
251-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
@param num_aicoredoc oninit_args_dump.The doc block documents
num_aicorebut the declared signatureint init_args_dump(Runtime &runtime, int device_id);only takes two parameters. This appears to be a leftover from the tensor-dump doc that wasn't fully cleaned up during the rename.📝 Proposed doc fix
/** * Initialize args dump device buffers. * * `@param` runtime Runtime instance to configure - * `@param` num_aicore Number of AICore instances (unused) * `@param` device_id Device ID for allocations * `@return` 0 on success, error code on failure */ int init_args_dump(Runtime &runtime, int device_id);🤖 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 251 - 258, The doc comment for init_args_dump still mentions a num_aicore parameter that no longer exists in the function signature. Update the documentation for init_args_dump in device_runner.h so the `@param` list matches the actual parameters Runtime &runtime and int device_id, and remove the stale num_aicore entry left over from the old tensor-dump wording.src/common/platform/shared/host/args_dump_collector.cpp (1)
411-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover
tensor_dump_*names in these renamed helpers.These static helpers now take
ArgsDump*types but still carry thetensor_dump_prefix, leaving a naming inconsistency in a PR whose stated goal is the tensor→args rename. Renaming them (and their call sites at Lines 646-648) keeps the subsystem naming uniform.♻️ Proposed rename
-static const char *tensor_dump_role_name(ArgsDumpRole role) { +static const char *args_dump_role_name(ArgsDumpRole role) { switch (role) { case ArgsDumpRole::INPUT: return "input"; case ArgsDumpRole::OUTPUT: return "output"; case ArgsDumpRole::INOUT: return "inout"; } return "unknown"; } -static const char *tensor_dump_stage_name(ArgsDumpStage stage) { +static const char *args_dump_stage_name(ArgsDumpStage stage) { switch (stage) { case ArgsDumpStage::BEFORE_DISPATCH: return "before_dispatch"; case ArgsDumpStage::AFTER_COMPLETION: return "after_completion"; } return "unknown"; } -static const char *tensor_dump_kind_name(ArgsDumpKind kind) { +static const char *args_dump_kind_name(ArgsDumpKind kind) { switch (kind) { case ArgsDumpKind::TENSOR: return "tensor"; case ArgsDumpKind::SCALAR: return "scalar"; } return "unknown"; }Corresponding call sites (Lines 646-648) would update to
args_dump_role_name/args_dump_stage_name/args_dump_kind_name.🤖 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/common/platform/shared/host/args_dump_collector.cpp` around lines 411 - 441, The helper names still use the old tensor prefix even though they now operate on ArgsDump types, so rename tensor_dump_role_name, tensor_dump_stage_name, and tensor_dump_kind_name to match the new args-dump terminology. Update the corresponding call sites in the same collector code to use the new names so the ArgsDump subsystem naming stays consistent throughout args_dump_collector.cpp.src/common/platform/shared/aicpu/args_dump_aicpu.cpp (1)
133-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate task-slot addressing logic; unused helper?
resolve_dump_args_task_slotcomputes the ring_id/slot/idx addressing, butset_dump_args_task_maskandget_dump_args_task_masksboth re-derive the same ring_id/slot/idx inline instead of calling it. This is fine functionally today (the three copies agree), but any future change to the pool constants or wrap logic could update one copy and silently miss the others, causing mismatched mask lookups.Consider having both call sites delegate to
resolve_dump_args_task_slotfor a single source of truth.♻️ Proposed consolidation
extern "C" void set_dump_args_task_mask(uint64_t task_id, ArgsDumpArgMask mask, ArgsDumpArgMask flags) { if (mask == ARGS_DUMP_ARG_MASK_NONE) { return; } if (!ensure_dump_args_mask_table()) { return; } - uint32_t ring_id = static_cast<uint32_t>(task_id >> 32); - if (ring_id >= ARGS_DUMP_MASK_POOL_MAX_RINGS) { - return; - } - uint32_t slot = static_cast<uint32_t>(task_id) & ARGS_DUMP_MASK_POOL_DEFAULT_SLOT_MASK; - uint32_t idx = (ring_id * ARGS_DUMP_MASK_POOL_MAX_SLOTS + slot) & (DUMP_TASK_MASK_TABLE_CAPACITY - 1); + uint32_t idx = 0; + if (!resolve_dump_args_task_slot(task_id, &idx)) { + return; + } for (uint32_t probe = 0; probe < DUMP_TASK_MASK_TABLE_CAPACITY; probe++) {Apply the analogous change in
get_dump_args_task_masks.Since I can't see the full file (lines 142-194 are elided as unchanged), please confirm whether
resolve_dump_args_task_slotis already used elsewhere; if it's entirely unused, it may also trigger an unused-static-function warning.Also applies to: 220-243, 245-276
🤖 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/common/platform/shared/aicpu/args_dump_aicpu.cpp` around lines 133 - 141, The task-slot indexing logic is duplicated, and the helper resolve_dump_args_task_slot should be used as the single source of truth. Update set_dump_args_task_mask and get_dump_args_task_masks to call resolve_dump_args_task_slot instead of re-deriving ring_id, slot, and idx inline, so the mask lookup behavior stays consistent if the pool constants or wrap logic change. Also verify whether resolve_dump_args_task_slot is referenced anywhere else in args_dump_aicpu.cpp; if not, either wire it into both call sites or remove it to avoid an unused-static-function warning.
🤖 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.
Inline comments:
In `@docs/dfx/args-dump.md`:
- Around line 833-834: Update the docs example text so it refers to the correct
JSON field name: in the args dump explanation, change the mention of tensors[]
to args[] to match the manifest example. Make this fix in the prose around the
dropped-args description so readers are directed to the same field name used by
the example, using the args dump section’s existing wording as the anchor.
- Around line 75-76: The documentation wording is using a worker handshake field
for profiling enablement, which conflicts with the runtime contract. Update the
`args-dump` text to refer directly to `KernelArgs` and its profiling-related
field(s) instead of `enable_profiling_flag` in the worker handshake, keeping the
description aligned with the host/device flow and the symbols used in the
runtime path.
In `@docs/dfx/dfx-buffer-capacity-audit.md`:
- Line 305: The appendix row is using outdated `DUMP_*` knob names instead of
the documented `PLATFORM_DUMP_*` names. Update the `args_dump` entry in the
audit appendix to match the same `PLATFORM_DUMP_RECORDS_PER_BUFFER` and
`PLATFORM_DUMP_BUFFERS_PER_THREAD` naming used elsewhere so the table stays
consistent.
In `@simpler_setup/tools/l0_swimlane.py`:
- Around line 305-311: The failure message in the args-dump check is partially
renamed and still contains stale tensor-dump wording. Update the RuntimeError
text in the no-new-outputs path of simpler_setup/tools/l0_swimlane.py so the
nested warning phrase matches args dump terminology throughout, keeping the
existing context around test_path.name and the skipped-dump explanation
consistent.
---
Nitpick comments:
In `@src/a5/platform/onboard/host/device_runner.h`:
- Around line 251-258: The doc comment for init_args_dump still mentions a
num_aicore parameter that no longer exists in the function signature. Update the
documentation for init_args_dump in device_runner.h so the `@param` list matches
the actual parameters Runtime &runtime and int device_id, and remove the stale
num_aicore entry left over from the old tensor-dump wording.
In `@src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp`:
- Around line 114-115: The section header comment in aicpu_executor.cpp is stale
and still says “Dump tensor state” even though the nearby Runtime cache comment
and try_dispatch_task usage now refer to args dump access. Update the header
comment near runtime_ in aicpu_executor.cpp so it matches the new args-dump
naming used in this rename PR and stays consistent with the surrounding code.
In `@src/common/platform/shared/aicpu/args_dump_aicpu.cpp`:
- Around line 133-141: The task-slot indexing logic is duplicated, and the
helper resolve_dump_args_task_slot should be used as the single source of truth.
Update set_dump_args_task_mask and get_dump_args_task_masks to call
resolve_dump_args_task_slot instead of re-deriving ring_id, slot, and idx
inline, so the mask lookup behavior stays consistent if the pool constants or
wrap logic change. Also verify whether resolve_dump_args_task_slot is referenced
anywhere else in args_dump_aicpu.cpp; if not, either wire it into both call
sites or remove it to avoid an unused-static-function warning.
In `@src/common/platform/shared/host/args_dump_collector.cpp`:
- Around line 411-441: The helper names still use the old tensor prefix even
though they now operate on ArgsDump types, so rename tensor_dump_role_name,
tensor_dump_stage_name, and tensor_dump_kind_name to match the new args-dump
terminology. Update the corresponding call sites in the same collector code to
use the new names so the ArgsDump subsystem naming stays consistent throughout
args_dump_collector.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27187ca8-4020-4d89-b964-36560504a3ea
📒 Files selected for processing (70)
docs/dfx/args-dump.mddocs/dfx/dfx-buffer-capacity-audit.mddocs/dfx/l2-swimlane-profiling.mddocs/dfx/pmu-profiling.mddocs/profiling-framework.mdsimpler_setup/tools/l0_swimlane.pysrc/a2a3/platform/include/aicore/aicore_profiling_state.hsrc/a2a3/platform/include/common/args_dump.hsrc/a2a3/platform/include/common/dep_gen.hsrc/a2a3/platform/include/common/kernel_args.hsrc/a2a3/platform/include/common/platform_config.hsrc/a2a3/platform/onboard/aicpu/CMakeLists.txtsrc/a2a3/platform/onboard/aicpu/kernel.cppsrc/a2a3/platform/onboard/host/CMakeLists.txtsrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/platform/onboard/host/device_runner.hsrc/a2a3/platform/shared/host/l2_swimlane_collector.cppsrc/a2a3/platform/sim/host/CMakeLists.txtsrc/a2a3/platform/sim/host/device_runner.cppsrc/a2a3/platform/sim/host/device_runner.hsrc/a2a3/runtime/host_build_graph/aicore/aicore_executor.cppsrc/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a2a3/runtime/host_build_graph/runtime/pto_runtime2_types.hsrc/a2a3/runtime/host_build_graph/runtime/runtime.hsrc/a2a3/runtime/tensormap_and_ringbuffer/aicore/aicore_executor.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/a5/platform/include/aicore/aicore_profiling_state.hsrc/a5/platform/include/common/args_dump.hsrc/a5/platform/include/common/dep_gen.hsrc/a5/platform/include/common/kernel_args.hsrc/a5/platform/include/common/platform_config.hsrc/a5/platform/include/common/pmu_profiling.hsrc/a5/platform/onboard/aicpu/CMakeLists.txtsrc/a5/platform/onboard/aicpu/kernel.cppsrc/a5/platform/onboard/host/CMakeLists.txtsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/platform/onboard/host/device_runner.hsrc/a5/platform/shared/host/l2_swimlane_collector.cppsrc/a5/platform/sim/host/CMakeLists.txtsrc/a5/platform/sim/host/device_runner.cppsrc/a5/platform/sim/host/device_runner.hsrc/a5/runtime/host_build_graph/aicore/aicore_executor.cppsrc/a5/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a5/runtime/host_build_graph/runtime/pto_runtime2_types.hsrc/a5/runtime/host_build_graph/runtime/runtime.hsrc/a5/runtime/tensormap_and_ringbuffer/aicore/aicore_executor.cppsrc/a5/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/common/platform/include/aicpu/args_dump_aicpu.hsrc/common/platform/include/common/scope_stats.hsrc/common/platform/include/host/args_dump_collector.hsrc/common/platform/include/host/buffer_pool_manager.hsrc/common/platform/include/host/profiler_base.hsrc/common/platform/include/host/profiling_copy.hsrc/common/platform/onboard/aicpu/spin_hint.hsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/shared/aicpu/args_dump_aicpu.cppsrc/common/platform/shared/host/args_dump_collector.cppsrc/common/platform/sim/host/device_runner_base.cppsrc/common/platform/sim/host/device_runner_base.hsrc/common/task_interface/data_type.htests/ut/cpp/CMakeLists.txt
indigo1973
force-pushed
the
dump_0702
branch
2 times, most recently
from
41124f0 to
7953b5b
Compare
indigo1973
changed the title
indigo1973
force-pushed
the
dump_0702
branch
2 times, most recently
from
031dfe8 to
2198c99
Compare
Finishes the tensor-dump -> args-dump naming migration (issue hw-native-sys#1252) by renaming both the internal implementation and the remaining public and example surfaces that the earlier passes (hw-native-sys#1072, hw-native-sys#1143) left untouched. Internal implementation: - Rename args-dump C++ types, headers, helpers, and CMake source paths from TensorDump* to ArgsDump* (collector, AICPU helpers, platform headers, DumpArgsLevel, tensor_dump_*.{h,cpp} -> args_dump_*). Public config field: - Rename CallConfig::enable_dump_tensor -> enable_dump_args across the C++ struct, the L3<->L2 wire codec (remote_wire.cpp and the Python remote_l3_protocol.py / worker.py decoders), the nanobind property, the host device_runner apply_call_config consumers, scene_test.py, the per_task_runtime_env example, and the chip-worker unit tests. The wire byte layout is unchanged (the field keeps its position); only the identifier is renamed. No compatibility alias is kept, so downstream callers (e.g. pypto) must migrate enable_dump_tensor -> enable_dump_args. Example + docs: - Rename the host_build_graph dump_tensor example (a2a3 + a5) to dump_args: directory, test module, TestDumpTensorExample -> TestDumpArgsExample, dump_tensor_orch.cpp -> dump_args_orch.cpp, and build_dump_tensor_graph -> build_dump_args_graph (including the copy reused by the a5 prepared_callable test). Update the args-dump doc links accordingly. - The TensorInfo metadata API (TensorInfo, set_tensor_info_to_task, add_task_with_tensor_info, make_tensor_info_from_tensor_arg) is left as-is: it describes tensor shape metadata specifically, not the general per-arg dump, so its naming is accurate. Also fix two stale comments the rename left behind: the "Dump tensor state" block comment in the a5 aicpu_executor (-> "Args dump state", to match the adjacent "args dump" wording) and the dump_args_init doc comment that described threads as dumping "tensors" rather than args. The tensor_dump.json negative assertions in test_args_dump.py are kept on purpose: they guard that the legacy filename is never emitted.
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.
Finishes the tensor-dump -> args-dump naming migration (issue #1252) by
renaming both the internal implementation and the remaining public and
example surfaces that the earlier passes (#1072, #1143) left untouched.
Internal implementation:
from TensorDump* to ArgsDump* (collector, AICPU helpers, platform
headers, DumpArgsLevel, tensor_dump_.{h,cpp} -> args_dump_).
Public config field:
C++ struct, the L3<->L2 wire codec (remote_wire.cpp and the Python
remote_l3_protocol.py / worker.py decoders), the nanobind property,
the host device_runner apply_call_config consumers, scene_test.py, the
per_task_runtime_env example, and the chip-worker unit tests. The wire
byte layout is unchanged (the field keeps its position); only the
identifier is renamed. No compatibility alias is kept, so downstream
callers (e.g. pypto) must migrate enable_dump_tensor -> enable_dump_args.
Example + docs:
dump_args: directory, test module, TestDumpTensorExample ->
TestDumpArgsExample, dump_tensor_orch.cpp -> dump_args_orch.cpp, and
build_dump_tensor_graph -> build_dump_args_graph (including the copy
reused by the a5 prepared_callable test). Update the args-dump doc
links accordingly.
add_task_with_tensor_info, make_tensor_info_from_tensor_arg) is left
as-is: it describes tensor shape metadata specifically, not the
general per-arg dump, so its naming is accurate.
Also fix two stale comments the rename left behind: the "Dump tensor
state" block comment in the a5 aicpu_executor (-> "Args dump state", to
match the adjacent "args dump" wording) and the dump_args_init doc
comment that described threads as dumping "tensors" rather than args.
The tensor_dump.json negative assertions in test_args_dump.py are kept
on purpose: they guard that the legacy filename is never emitted.