Repository navigation
Conversation
`src/common/{host_build_graph,tensormap_and_ringbuffer}/tensor.h` sits on every
kernel translation unit's include path: each runtime's `runtime/tensor.h` umbrella
includes it to supply that runtime's working `Tensor`, and kernels name it through
the umbrella's `TaskTensor` alias. Its last line was a runtime-internal container,
using EntryArgsStorage =
TaskArgsTpl<Tensor, uint64_t, CHIP_MAX_TENSOR_ARGS, CHIP_MAX_SCALAR_ARGS>;
and that line was the file's only reason to include `task_interface/task_args.h`,
and through it `task_interface/buffer.h`. So every kernel pulled in the L3+
argument surface it has no use for, plus the address-free wire `Tensor` that
buffer.h declares at global scope. `EntryArgsStorage` has no kernel consumer at
all — the readers are `runtime/runtime.h`, `runtime/types.h`,
`runtime/shared/runtime.cpp`, `host/runtime_maker.cpp`, and for tmr
`aicpu/aicpu_executor.cpp`.
It moves to a sibling `entry_args.h` per runtime, which includes `tensor.h` and
`task_args.h` for itself. The eight runtime headers that name the alias include
that instead of the umbrella; the six .cpp consumers already reach it through
`runtime.h`. No declaration changes and nothing else moves.
The wire `Tensor` and a runtime's `Tensor` share a name in different scopes, so
while the edge existed a kernel could not write `using Tensor =
simpler::hbg::Tensor;` — the alias was a conflicting redeclaration of buffer.h's
struct. Both umbrellas now accept it. Confirmed against a control that restores
the include by hand and still fails, so the check answers to the edge rather than
to the alias.
Verified with the full product build — both arches x both runtimes x sim and
onboard, host, aicpu and aicore artifacts all rebuilt — cpput 119/119, and both
sim scene-test sweeps in the shape `_st-sim-{a2a3,a5}.yml` runs them: a2a3sim 60
passed / 8 skipped, a5sim 53 passed, each reconciling against `--collect-only`.
The scene-test corpus's incores were also compiled with `--manual include` on both
arches, which is the coverage hw-native-sys#1974 lacked when it renamed a type only manual-only
cases used. That reports six failures per arch, all pre-existing on this base and
all carrying hw-native-sys#1974's signature (`no member named 'tmr' in namespace 'simpler'`) in
four files: the two qwen `vendor/paged_attention_cce` entries hw-native-sys#2023 fixes, and the
two `spmd_paged_attention` files it reports separately. No failure names
`entry_args.h`, `task_args.h`, `buffer.h`, a conflicting `Tensor` declaration or an
ambiguous call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
💤 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 ChangesEntry Argument Header Split
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR relocates a runtime-only type out of kernel-facing headers without changing declarations or behavior. Reported builds and tests are clean, and no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 10 files. (1 skipped: 1 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 |
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Aug 27, 2026
`task_args.h` bundled two things at different layers. The generic argument
container — `TaskArgsTpl`, its tag mixin, and the `ChipStorageTaskArgs` alias whose
element is `ChipTensor` — is needed by every runtime and orchestration translation
unit. Alongside it sat the L3+ form, whose element is the self-describing `Tensor`
from buffer.h: the `TaskArgs` builder, `TaskArgsView`, the mailbox blob codec, and
`validate_submit_args`. Because those need buffer.h, the header included it, and
`buffer.h` declares a `Tensor` at **global scope**.
buffer.h has exactly one includer in src/ — that line — so the effect was that every
orchestration translation unit saw the L3+ wire type. Measured on the pre-change
tree, this chain:
orchestration_api.h -> runtime/graph_cache.h -> common/{hbg}/graph_cache.h
-> runtime/types.h -> task_args.h -> task_interface/buffer.h
`types.h` needs the container because `Arg : TaskArgsTpl<TensorRef, …>` is the submit
surface orchestration builds on; it has no use for the wire form.
The L3+ half moves to `task_args_wire.h`, which includes `task_args.h` and buffer.h.
Its consumers are the layer that actually handles that form: the host orchestrator
under `src/common/hierarchical/`, the Python bindings, the platform host code that
decodes a blob into `ChipStorageTaskArgs`, and the cpput case that round-trips a blob.
Eleven includes change; nothing else moves and no declaration changes. `task_args.h`
keeps its name, so the fourteen runtime and orchestration includers are untouched, and
carries a pointer to where `TaskArgs` went — a reader grepping the obvious file for the
obvious name should not come up empty.
`ChipStorageTaskArgs` stays on the generic side by the same criterion that drove the
split: its element is `ChipTensor` from task_interface/tensor.h, not the wire
`Tensor`, so it needs no buffer.h. It is also what `runtime.h` and
`orchestration_api.h` include the header for.
The consequence is that `Tensor` is no longer taken inside a kernel or orchestration
source. hw-native-sys#2032 established that for kernels; this closes the orchestration half, which
is the larger one — `using Tensor = simpler::hbg::Tensor;` in an orchestration
translation unit failed with a conflicting declaration before this change and
compiles after. Verified in both directions: a control that pulls in
`task_args_wire.h` by hand on the changed tree still fails, so the check answers to
the include edge rather than to the alias. Both runtimes' orchestration paths were
probed with the include flags taken from `compile_commands.json` — `build_config.py`'s
`include_dirs` are relative to the runtime directory and do not carry `src/common`,
which the platform CMakeLists adds, so reading the build config alone gives the wrong
answer.
Also verified with the full product build across both arches x both runtimes x
sim/onboard, cpput 119/119 from a clean build directory, both sim scene-test sweeps in
the shape `_st-sim-{a2a3,a5}.yml` runs them (a2a3sim 60 passed / 8 skipped, a5sim 53
passed, each reconciling against `--collect-only`), and the whole scene-test corpus's
incores compiled with `--manual include` on both arches — zero failures, where the same
sweep reported six per arch before hw-native-sys#2027 landed, so it is sensitive rather than vacuous.
Clean is the operative word for cpput: an incremental build reuses object files and
dependency information, so it does not recompile a test whose include set this change
invalidates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Aug 27, 2026
`task_args.h` bundled two things at different layers. The generic argument
container — `TaskArgsTpl`, its tag mixin, and the `ChipStorageTaskArgs` alias whose
element is `ChipTensor` — is needed by every runtime and orchestration translation
unit. Alongside it sat the L3+ form, whose element is the self-describing `Tensor`
from buffer.h: the `TaskArgs` builder, `TaskArgsView`, the mailbox blob codec, and
`validate_submit_args`. Because those need buffer.h, the header included it, and
`buffer.h` declares a `Tensor` at **global scope**.
buffer.h has exactly one includer in src/ — that line — so the effect was that every
orchestration translation unit saw the L3+ wire type. Measured on the pre-change
tree, this chain:
orchestration_api.h -> runtime/graph_cache.h -> common/{hbg}/graph_cache.h
-> runtime/types.h -> task_args.h -> task_interface/buffer.h
`types.h` needs the container because `Arg : TaskArgsTpl<TensorRef, …>` is the submit
surface orchestration builds on; it has no use for the wire form.
The L3+ half moves to `task_args_wire.h`, which includes `task_args.h` and buffer.h.
Its consumers are the layer that actually handles that form: the host orchestrator
under `src/common/hierarchical/`, the Python bindings, the platform host code that
decodes a blob into `ChipStorageTaskArgs`, and the cpput case that round-trips a blob.
Eleven includes change; nothing else moves and no declaration changes. `task_args.h`
keeps its name, so the fourteen runtime and orchestration includers are untouched, and
carries a pointer to where `TaskArgs` went — a reader grepping the obvious file for the
obvious name should not come up empty.
`ChipStorageTaskArgs` stays on the generic side by the same criterion that drove the
split: its element is `ChipTensor` from task_interface/tensor.h, not the wire
`Tensor`, so it needs no buffer.h. It is also what `runtime.h` and
`orchestration_api.h` include the header for.
The consequence is that `Tensor` is no longer taken inside a kernel or orchestration
source. #2032 established that for kernels; this closes the orchestration half, which
is the larger one — `using Tensor = simpler::hbg::Tensor;` in an orchestration
translation unit failed with a conflicting declaration before this change and
compiles after. Verified in both directions: a control that pulls in
`task_args_wire.h` by hand on the changed tree still fails, so the check answers to
the include edge rather than to the alias. Both runtimes' orchestration paths were
probed with the include flags taken from `compile_commands.json` — `build_config.py`'s
`include_dirs` are relative to the runtime directory and do not carry `src/common`,
which the platform CMakeLists adds, so reading the build config alone gives the wrong
answer.
Also verified with the full product build across both arches x both runtimes x
sim/onboard, cpput 119/119 from a clean build directory, both sim scene-test sweeps in
the shape `_st-sim-{a2a3,a5}.yml` runs them (a2a3sim 60 passed / 8 skipped, a5sim 53
passed, each reconciling against `--collect-only`), and the whole scene-test corpus's
incores compiled with `--manual include` on both arches — zero failures, where the same
sweep reported six per arch before #2027 landed, so it is sensitive rather than vacuous.
Clean is the operative word for cpput: an incremental build reuses object files and
dependency information, so it does not recompile a test whose include set this change
invalidates.
ChaoWao
added a commit
that referenced
this pull request
Aug 27, 2026
…on sources (#2044) Both runtime umbrellas exported their working tensor as `TaskTensor`, and 693 files in the scene-test corpus named it. The prefix carried no meaning: a kernel has exactly one tensor type, the one its payload elements are, and the umbrella exists precisely so a source compiled under both runtimes need not pick one. The name was a workaround — `Tensor` was taken inside those translation units by `task_interface/buffer.h`, which declares the address-free L3+ wire form at global scope, so the obvious alias was a conflicting redeclaration. Nothing takes it any more. #2032 lifted `EntryArgsStorage` out of the kernel-facing `common/{host_build_graph,tensormap_and_ringbuffer}/tensor.h`, cutting the kernel path to buffer.h; #2038 split `TaskArgs` and the blob codec into `task_args_wire.h`, cutting the orchestration path, which was the larger one. So the four umbrellas now export using Tensor = simpler::{hbg,tmr}::Tensor; and the 5657 occurrences across 693 files follow. The sweep is word-boundary anchored, so `ChipTensor` and `GlobalTensor` are untouched, and it ran over `git grep -Il` output so build artifacts were not rewritten. Verified beforehand that no C++ source under examples/ or tests/st/ already used a bare `Tensor`: the pattern finds 81 lines when Python and Markdown are included — `torch.Tensor`, the Python API, prose — and zero in `*.cpp`/`*.h`/`*.hpp`. The 234 files clang-format then wanted to change are a consequence of the rename, not pre-existing drift: a four-character-shorter name lets it rejoin lines that were wrapped. Checked by restoring one file's pre-rename content in place and confirming it is format-clean, where the renamed one is not. Formatting them makes the diff smaller overall. Two under `kernels/vendor/` are left alone, matching the clang-format hook's own exclusion. `tests/lint/check_kernel_wire_isolation.py` now holds the separation the name depends on. It checks the include *edge* rather than reachability, because pre-commit sees only the staged set and the file that would break reachability is typically a shared header such as `runtime/types.h`, not something under a kernel path: any file outside an allowlist that includes `task_interface/buffer.h` or `task_args_wire.h` fails. The allowlist is the twelve places that genuinely handle the L3+ form — the host orchestrator, the Python bindings, the platform host code that decodes a mailbox blob, and their unit tests — so it also documents who they are. Adding an entry stays possible and is a reviewable diff; adding one to a header kernels reach is the defect this surfaces. Exercised three ways: zero findings over all 1560 tracked C++ files, one finding on a probe that adds the include, and no finding on a probe naming `ring_buffer.h`, `aligned_buffer.h`, `kernel_pop_stack_buffer.h` and `task_args.h`. Verified with the full product build across both arches x both runtimes x sim/onboard, cpput 122/122 from a clean build directory, both sim scene-test sweeps in the shape `_st-sim-{a2a3,a5}.yml` runs them (a2a3sim 61 passed / 8 skipped, a5sim 54 passed, zero failures), and — the gate that actually covers a corpus-wide kernel rename — every scene test's incores compiled with `--manual include` on both arches, zero failures. That sweep reported six failures per arch before #2027 landed its fixes, so it is sensitive rather than vacuous. All of it re-run after the formatting pass.
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.
The defect
src/common/{host_build_graph,tensormap_and_ringbuffer}/tensor.hsits on every kernel translation unit's include path — each runtime'sruntime/tensor.humbrella includes it to supply that runtime's workingTensor, which kernels name through the umbrella'sTaskTensoralias. Its last line was a runtime-internal container:That line was the file's only reason to include
task_interface/task_args.h, and through ittask_interface/buffer.h:So every kernel pulled in the L3+ argument surface it has no use for, plus the address-free wire
Tensorthatbuffer.hdeclares at global scope.task_args.hneedingbuffer.his legitimate — it declaresusing TaskArgs = TaskArgsTpl<Tensor, ...>and the blob codec, which is the L3+ surface. The defect is that the edge into it existed at all.EntryArgsStoragehas no kernel consumer. Measured, the readers are all runtime-side:runtime/runtime.h×4orch_args_storage_member +get_orch_args()return typeruntime/types.h×4create_from_entry_storage(const EntryArgsStorage &)runtime/shared/runtime.cpp×4get_orch_args()definition,set_orch_argsadoptionhost/runtime_maker.cpp×2create_from_entry_storagecallaicpu/aicpu_executor.cpp×2 (tmr only)The change
It moves to a sibling
entry_args.hper runtime, which includestensor.handtask_args.hfor itself. The eight runtime headers that name the alias include that instead of the umbrella; the six.cppconsumers already reach it throughruntime.h, so they are untouched. No declaration changes and nothing else moves — 13 files, +89/−18.task_interface/arg_direction.hstays in bothtensor.hfiles. It is unused there once the alias leaves, but it is a<cstdint>-only leaf that cannot reachbuffer.h, it does not participate in this defect, and it exports theMAX_TENSOR_ARGS/MAX_SCALAR_ARGSmacros — removing an include that 688 files see transitively is a separate change with its own blast radius.Why it matters beyond hygiene
The wire
Tensorand a runtime'sTensorshare a name in different scopes, so while that edge existed a kernel could not write the obvious alias:That is why the umbrellas export the awkward
TaskTensorrather thanTensor. Both umbrellas now acceptusing Tensor = simpler::{hbg,tmr}::Tensor;. This PR does not rename anything (that would be 3286 occurrences across 688 files, and the name would then rest on an invariant nothing enforces) — it only removes the reason the rename was impossible.docs/buffer-abi.md's "Three types" section records the new invariant, since it is exactly the kind a future#includecan silently undo.Testing
-fsyntax-onlyspot check missedaicpu/aicpu_executor.cpp)_st-sim-a2a3.ymlmain sweep, verbatim shape--collect-only's 68 selected_st-sim-a5.ymlmain sweep, verbatim shapeThe acceptance check is a probe TU that includes the umbrella and aliases
Tensor. It compiles now; a control that restores thetask_args.hinclude by hand on the same tree still fails with the error above — so the check answers to the edge rather than to the alias.Pre-existing failures this surfaced, and why they are not this PR's
The whole scene-test corpus's incores were also compiled with
--manual includeon both arches — the coverage #1974 lacked when it renamed a type that only manual-only cases use. That reports six failures per arch, all pre-existing on this base, all carrying #1974's signature in four files:examples/{a2a3,a5}/…/qwen3_14b_decode/kernels/vendor/paged_attention_cce/{tiling,attention_rope}/entry.cpp— fixed by Name TaskTensor in the qwen vendored kernels so both umbrellas compile them #2023, not in this basetests/st/{a2a3,a5}/…/spmd_paged_attention/kernels/{mix/paged_attention_parallel.cpp,orchestration/spmd_paged_attention_orch.cpp}— reported separately in Name TaskTensor in the qwen vendored kernels so both umbrellas compile them #2023, still openNegative evidence rather than assertion: grepping both logs for
entry_args|task_args\.h|buffer\.h|conflicting declaration|ambiguousmatches twice, and both are a CANNunused variablewarning inkernel_pop_stack_buffer.h. No failure names a changed header, a conflictingTensor, or an ambiguous call.Note that
scene_test_compileis a cache warmer by its own docstring and exits 0 on a compile error, logging[ERROR] [Incore] Compilation failed:instead — the log is the signal, not the status.🤖 Generated with Claude Code