Skip to content

test(simt): unify scatter kernel on templated MSCATTER, drop __CPU_SIM fork#1275

Merged
ChaoZheng109 merged 1 commit into
hw-native-sys:mainfrom
indigo1973:fix/simt-scatter-unify-mscatter
Jul 6, 2026
Merged

test(simt): unify scatter kernel on templated MSCATTER, drop __CPU_SIM fork#1275
ChaoZheng109 merged 1 commit into
hw-native-sys:mainfrom
indigo1973:fix/simt-scatter-unify-mscatter

Conversation

@indigo1973

Copy link
Copy Markdown
Contributor

What

Remove the #ifdef __CPU_SIM fork in the SIMT element-scatter ST kernel
(kernel_simt_scatter.cpp)
so both the CPU simulator and a5 onboard issue the same instruction:

MSCATTER<Coalesce::Elem, ScatterAtomicOp::None, ScatterOOB::Skip>(outGlobal, srcTile, idxTile);

Why

The fork existed because pto-isa previously gated the templated MSCATTER
overloads behind PTO_NPU_ARCH_A5 only — they weren't visible to the CPU
simulator, so the sim path had to fall back to the non-templated form
(pto-isa#164). The templated overloads were later opened to __CPU_SIM as
well as PTO_NPU_ARCH_A5 (pto-isa#166), so the explicit templated call now
compiles and runs identically on both backends.

This branch is based on current main, whose pto_isa.pin (83d01313) is a
descendant of 016396b5 (bumped in #1156, the pto-isa#166 mechanism). At that
pin the __CPU_SIM guard on the templated overload is present, which is what
lets the sim compile the explicit call.

Caveat — non-templated form is still not portable

Only the templated overload was unified. The non-templated
MSCATTER(dst, src, idx) still picks each backend's own default:
Coalesce::Elem on CPU sim (pto/cpu/MScatter.hpp) vs Coalesce::Row on
a5 onboard (pto/npu/a5/MScatter.hpp). So the explicit
MSCATTER<Coalesce::Elem, ...> is the only instruction that behaves the same
on both — hence keeping the template arguments rather than relying on the
non-templated default.

Testing

  • --platform a5sim: 1 passed (run against the branch's pinned pto-isa
    83d01313, i.e. the same pin CI uses — no --pto-isa-commit override).
  • Onboard a5 call site is unchanged by this cleanup (it already used this
    exact instruction), so a5 behavior is unaffected. An a5 onboard rerun via
    CI's st-onboard-a5 closes the loop — not available on the current a2a3 dev
    box.

Relationship to #1160

Supersedes #1160 (same change, by @ChaoZheng109). That PR's branch was cut
before #1156 landed and still pinned the pre-pto-isa#166 revision
ddafa8da, where the templated overload is not visible under __CPU_SIM
so its st-sim-a5 failed to compile ('Coalesce' was not declared in this scope). This branch is the same one-line cleanup, rebased onto current main
so it builds against the correct pin.

Closes #1159

… MSCATTER

The simt_basic scatter kernel forked the scatter call on __CPU_SIM
because pto-isa previously gated the templated MSCATTER overloads behind
PTO_NPU_ARCH_A5 only, so the CPU simulator had to use the non-templated
form. The pinned pto-isa (bumped to 016396b5 in hw-native-sys#1156, the pto-isa#166
mechanism) now exposes the templated overloads to __CPU_SIM as well, so
both backends can call the same explicit
MSCATTER<Coalesce::Elem, ScatterAtomicOp::None, ScatterOOB::Skip>.

Note the non-templated MSCATTER is still not portable: CPU sim defaults
to Coalesce::Elem while a5 onboard defaults to Coalesce::Row, so the
explicit templated form is the only instruction unified across both.

Verified on --platform a5sim. Onboard call site is unchanged.

Closes hw-native-sys#1159
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e132fc4-7a02-4ec5-8aec-beca7a59a1e9

📥 Commits

Reviewing files that changed from the base of the PR and between 57f82d5 and f06f297.

📒 Files selected for processing (1)
  • tests/st/a5/tensormap_and_ringbuffer/simt_basic/kernels/aiv/kernel_simt_scatter.cpp

📝 Walkthrough

Walkthrough

A conditional #ifdef __CPU_SIM branch in a test kernel's scatter operation was removed. The scatter call now unconditionally uses the templated MSCATTER<Coalesce::Elem, ScatterAtomicOp::None, ScatterOOB::Skip> form for both simulation and onboard builds, with updated comments.

Changes

Scatter Kernel Cleanup

Layer / File(s) Summary
Unify MSCATTER call
tests/st/a5/tensormap_and_ringbuffer/simt_basic/kernels/aiv/kernel_simt_scatter.cpp
Removes the __CPU_SIM branching and non-templated MSCATTER call, using the explicit templated MSCATTER call unconditionally with updated comments.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Poem

A fork removed, one path remains,
No more branching, fewer pains,
One MSCATTER call to rule them all,
Sim and onboard heed its call,
This rabbit hops with code so clean! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: unifying the scatter kernel on templated MSCATTER and removing the __CPU_SIM fork.
Description check ✅ Passed The description is directly about the same kernel cleanup and explains the templated MSCATTER unification.
Linked Issues check ✅ Passed The code change matches #1159 by dropping the CPU-sim fork and using the shared templated MSCATTER call.
Out of Scope Changes check ✅ Passed The patch is narrowly scoped to the requested scatter-kernel cleanup with no unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request simplifies the simt_scatter_impl kernel by removing the conditional compilation fork (#ifdef __CPU_SIM) for the MSCATTER instruction. Both the CPU simulation and onboard backends now consistently use the templated MSCATTER overload with Coalesce::Elem configuration. There are no review comments, so no further feedback is provided.

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.

@ChaoZheng109 ChaoZheng109 merged commit f35d47d into hw-native-sys:main Jul 6, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Health] simt_basic scatter kernel: drop the __CPU_SIM fork — use one templated MSCATTER across sim and onboard

2 participants