Update: enable A5 SDMA workspace by default - #1615
Open
doraemonmj wants to merge 1 commit into
Open
Conversation
- Provision the pinned PTO-ISA SDMA workspace in every A5 onboard build. - Keep URMA as the explicit alternative for the single communication workspace. - Run the A5 SDMA demo by default and remove the obsolete overlay guide.
📝 WalkthroughWalkthroughA5 onboard runtime builds now enable SDMA workspace provisioning by default. URMA remains an explicit alternative. PTO-ISA resolution and embedding are unconditional for A5, and tests, examples, and documentation reflect the new behavior. ChangesA5 workspace backend
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/README.md`:
- Around line 21-33: Gate the demo’s test requirements in README.md to the SDMA
backend, and update kernel_sdma_tget_async.cpp to validate that the workspace
backend is SDMA before calling SdmaTget. Preserve the existing A5 platform and
two-die requirements while preventing execution when the workspace is URMA.
In
`@examples/a5/tensormap_and_ringbuffer/urma_deferred_completion_demo/test_urma_deferred_completion_demo.py`:
- Line 217: Update the skip reason in the URMA deferred completion test to state
that SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON must be set both before rebuilding and
when running pytest, so the test-time environment requirement is explicit.
In `@simpler_setup/runtime_builder.py`:
- Around line 201-205: Update the `_build_cache_stamp` documentation in
`_resolve_build_pto_isa_commit` to state that both A2A3 and A5 onboard builds
include the PTO-ISA commit. In `src/a5/platform/onboard/host/CMakeLists.txt`
lines 126-131, replace the SDMA-specific cache explanation with wording that
also covers explicit URMA builds embedding PTO-ISA headers.
🪄 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 Plus
Run ID: 7060565c-d42f-489b-93a5-39294f9e730c
📒 Files selected for processing (16)
docs/README.mddocs/a5-sdma-overlay.mddocs/capability-survey.mddocs/comm-domain.mdexamples/a5/tensormap_and_ringbuffer/README.mdexamples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/README.mdexamples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/kernels/aiv/kernel_sdma_tget_async.cppexamples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/test_sdma_async_completion_demo.pyexamples/a5/tensormap_and_ringbuffer/urma_deferred_completion_demo/README.mdexamples/a5/tensormap_and_ringbuffer/urma_deferred_completion_demo/kernels/aiv/kernel_urma_tget_async.cppexamples/a5/tensormap_and_ringbuffer/urma_deferred_completion_demo/test_urma_deferred_completion_demo.pysimpler_setup/runtime_builder.pysimpler_setup/runtime_compiler.pysrc/a5/platform/onboard/host/CMakeLists.txtsrc/a5/platform/onboard/host/comm_hccl.cpptests/ut/py/test_runtime_builder.py
💤 Files with no reviewable changes (3)
- docs/a5-sdma-overlay.md
- examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/test_sdma_async_completion_demo.py
- docs/README.md
Comment on lines
+21
to
33
| ## Requirements | ||
|
|
||
| Unlike its a2a3 namesake, the a5 demo needs the PTO async-SDMA workspace | ||
| compiled into the host runtime: | ||
| The a5 host runtime includes the PTO async-SDMA workspace by default: | ||
|
|
||
| | Gate | Effect | | ||
| | ---- | ------ | | ||
| | `@pytest.mark.platforms(["a5"])` | deselected on any other `--platform` | | ||
| | `@pytest.mark.device_count(2)` | needs two dies | | ||
| | `@pytest.mark.skipif(...)` | skipped unless `SIMPLER_ENABLE_PTO_SDMA_WORKSPACE` is `1` / `ON` / `TRUE` / `YES` | | ||
|
|
||
| The CMake option defaults `OFF`, so a stock build skips this test even on a5 | ||
| hardware — **a green CI run says nothing about SDMA completion on a5.** | ||
|
|
||
| ```bash | ||
| SIMPLER_ENABLE_PTO_SDMA_WORKSPACE=ON pip install --no-build-isolation -e . | ||
| SIMPLER_ENABLE_PTO_SDMA_WORKSPACE=ON \ | ||
| pytest examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo \ | ||
| pytest examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo \ | ||
| --platform a5 --device 0-1 | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd -t f 'test_sdma_async_completion_demo.py' examples/a5/tensormap_and_ringbuffer | head -n 1)"
test -n "$test_file"
rg -n -C 6 'skipif|PTO_(URMA|SDMA)_WORKSPACE|platforms|device_count|run\(' "$test_file"
rg -n -C 8 'SIMPLER_ENABLE_PTO_(URMA|SDMA)_WORKSPACE|domain_workspace_addr|workSpace' \
src/a5/platform/onboard/host/CMakeLists.txt \
src/a5/platform/onboard/host/comm_hccl.cppRepository: hw-native-sys/simpler
Length of output: 26072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd -t f 'test_sdma_async_completion_demo.py' examples/a5/tensormap_and_ringbuffer | head -n 1)"
kernel_file="examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/kernels/aiv/kernel_sdma_tget_async.cpp"
readme_file="examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/README.md"
printf '%s\n' '--- test file ---'
wc -l "$test_file"
sed -n '1,215p' "$test_file"
printf '%s\n' '--- kernel ---'
wc -l "$kernel_file"
sed -n '1,125p' "$kernel_file"
printf '%s\n' '--- README ---'
cat -n "$readme_file"
printf '%s\n' '--- backend option propagation ---'
rg -n -C 5 'SIMPLER_ENABLE_PTO_(URMA|SDMA)_WORKSPACE|PTO.*WORKSPACE|RuntimeBuilder|host_runtime' \
--glob '!**/build/**' --glob '!**/node_modules/**' .Repository: hw-native-sys/simpler
Length of output: 1928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/test_sdma_async_completion_demo.py").read_text()
kernel = Path("examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/kernels/aiv/kernel_sdma_tget_async.cpp").read_text()
cmake = Path("src/a5/platform/onboard/host/CMakeLists.txt").read_text()
comm = Path("src/a5/platform/onboard/host/comm_hccl.cpp").read_text()
checks = {
"test_has_platform_gate": bool(re.search(r"`@pytest`\.mark\.platforms\(\[\"a5\"\]\)", test)),
"test_has_device_count_gate": bool(re.search(r"`@pytest`\.mark\.device_count\(2\)", test)),
"test_has_backend_gate": bool(re.search(r"skipif|PTO_(?:URMA|SDMA)_WORKSPACE", test)),
"kernel_guard_checks_workspace": bool(re.search(r"nranks\s*!=\s*2\s*\|\|\s*comm_ctx->workSpace\s*==\s*0", kernel)),
"kernel_calls_sdma_tget": "SdmaTget" in kernel,
"cmake_makes_sdma_default": bool(re.search(
r"if\s*\(\s*SIMPLER_ENABLE_PTO_URMA_WORKSPACE\s*\).*?"
r"set\s*\(\s*SIMPLER_ENABLE_PTO_SDMA_WORKSPACE\s+OFF\s*\).*?"
r"else\s*\(\).*?"
r"set\s*\(\s*SIMPLER_ENABLE_PTO_SDMA_WORKSPACE\s+ON\s*\)",
cmake, re.S)),
"urma_assigns_same_workspace": bool(re.search(
r"h->host_ctx\.workSpace\s*=\s*reinterpret_cast<uint64_t>\(h->urma_workspace->GetWorkspaceAddr\(\)\)",
comm)),
"sdma_assigns_same_workspace": bool(re.search(
r"h->host_ctx\.workSpace\s*=\s*reinterpret_cast<uint64_t>\(h->sdma_workspace->GetWorkspaceAddr\(\)\)",
comm)),
}
for name, value in checks.items():
print(f"{name}={value}")
PYRepository: hw-native-sys/simpler
Length of output: 1928
Gate the demo to the SDMA backend.
When SIMPLER_ENABLE_PTO_URMA_WORKSPACE is enabled, CommContext::workSpace contains a valid URMA workspace, but the kernel still calls SdmaTget. Add an SDMA backend gate to the test and validate the backend before using SdmaTget in the kernel.
📍 Affects 2 files
examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/README.md#L21-L33(this comment)examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/kernels/aiv/kernel_sdma_tget_async.cpp#L47-L48
🤖 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 `@examples/a5/tensormap_and_ringbuffer/sdma_async_completion_demo/README.md`
around lines 21 - 33, Gate the demo’s test requirements in README.md to the SDMA
backend, and update kernel_sdma_tget_async.cpp to validate that the workspace
backend is SDMA before calling SdmaTget. Preserve the existing A5 platform and
two-die requirements while preventing execution when the workspace is URMA.
| not _urma_workspace_enabled(), | ||
| reason="URMA workspace overlay not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON to run). " | ||
| "See docs/a5-sdma-overlay.md (#1315).", | ||
| reason="URMA workspace backend not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON before rebuilding).", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mention the test-time environment requirement.
The skip condition reads SIMPLER_ENABLE_PTO_URMA_WORKSPACE when pytest runs. A user who sets it only before rebuilding still gets a skipped test. State that the variable must be set both when rebuilding and when running pytest.
Proposed wording
- reason="URMA workspace backend not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON before rebuilding).",
+ reason="URMA workspace backend not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON before rebuilding and when running pytest).",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reason="URMA workspace backend not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON before rebuilding).", | |
| reason="URMA workspace backend not enabled (set SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON before rebuilding and when running pytest).", |
🤖 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
`@examples/a5/tensormap_and_ringbuffer/urma_deferred_completion_demo/test_urma_deferred_completion_demo.py`
at line 217, Update the skip reason in the URMA deferred completion test to
state that SIMPLER_ENABLE_PTO_URMA_WORKSPACE=ON must be set both before
rebuilding and when running pytest, so the test-time environment requirement is
explicit.
Comment on lines
201
to
+205
| def _resolve_build_pto_isa_commit(self) -> str: | ||
| """Return the pinned pto-isa commit baked into this build. | ||
|
|
||
| When host code embeds pto-isa headers (a2a3 onboard, or a5 onboard | ||
| with the SDMA or URMA overlay ON), a pto-isa bump must invalidate that build's | ||
| When host code embeds pto-isa headers (a2a3 or a5 onboard), a pto-isa | ||
| bump must invalidate that build's |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the PTO-ISA cache documentation.
The implementation now covers both A5 backends and both onboard embedding platforms. The current comments retain narrower old behavior.
simpler_setup/runtime_builder.py#L201-L205: Update the_build_cache_stamptext at Line 225. It must state that A2A3 and A5 onboard builds include the PTO-ISA commit.src/a5/platform/onboard/host/CMakeLists.txt#L126-L131: Replace the SDMA-specific explanation. The cache key also protects explicit URMA builds that embed PTO-ISA headers.
📍 Affects 2 files
simpler_setup/runtime_builder.py#L201-L205(this comment)src/a5/platform/onboard/host/CMakeLists.txt#L126-L131
🤖 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 `@simpler_setup/runtime_builder.py` around lines 201 - 205, Update the
`_build_cache_stamp` documentation in `_resolve_build_pto_isa_commit` to state
that both A2A3 and A5 onboard builds include the PTO-ISA commit. In
`src/a5/platform/onboard/host/CMakeLists.txt` lines 126-131, replace the
SDMA-specific cache explanation with wording that also covers explicit URMA
builds embedding PTO-ISA headers.
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
Validation
python -m pytest tests/ut/py/test_runtime_builder.py -q(45 passed)