Refactor: L3-L2 orch comm data path redesign#1291
Open
ccyywwen wants to merge 5 commits into
Open
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughAdds two new documentation files describing a refactor of the L3↔L2 orchestrator communication data path: a design document and a detailed implementation plan. Both specify removing the dedicated child-side shared-memory service, shifting steady-state payload/counter execution to the L3 parent, and preserving existing descriptor/queue ABI. ChangesL3-L2 Data Path Refactor Documentation
Estimated code review effort: 2 (Simple) | ~15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/l3-l2-comm-data-path-refactor-impl.md (1)
386-396: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep the buffer failure contract explicit. If parent-contiguous spans are added here, preserve the current pre-submit
ValueErrorbehavior and no-poison path for unsupported, non-contiguous, and non-writable inputs so the API stays stable.🤖 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 `@docs/l3-l2-comm-data-path-refactor-impl.md` around lines 386 - 396, The buffer-input expansion in this section should keep the existing failure contract intact. Update the primitive/buffer resolution path so any newly accepted parent-contiguous span still raises ValueError before submit for unsupported, non-contiguous, or non-writable inputs, and ensure the no-poison behavior remains unchanged. Use the existing primitive staging/resolution flow described here to gate acceptance, and preserve the current runtime Tensor path while broadening only when the buffer checks are explicit and stable.
🤖 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/l3-l2-comm-data-path-refactor-impl.md`:
- Around line 416-420: Clarify the SIGNAL_NOTIFY.Add overflow behavior in the
comm data path spec, since the current description only says to load, add, and
store an int32_t result. Update the SIGNAL_NOTIFY section to explicitly state
whether the addition should wrap, saturate, or fail on overflow, and make the
wording consistent with the parent implementation behavior so readers can
implement the add operation unambiguously.
---
Nitpick comments:
In `@docs/l3-l2-comm-data-path-refactor-impl.md`:
- Around line 386-396: The buffer-input expansion in this section should keep
the existing failure contract intact. Update the primitive/buffer resolution
path so any newly accepted parent-contiguous span still raises ValueError before
submit for unsupported, non-contiguous, or non-writable inputs, and ensure the
no-poison behavior remains unchanged. Use the existing primitive
staging/resolution flow described here to gate acceptance, and preserve the
current runtime Tensor path while broadening only when the buffer checks are
explicit and stable.
🪄 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: b9bbdc22-c02b-4476-a419-652417357517
📒 Files selected for processing (2)
docs/l3-l2-comm-data-path-refactor-design.mddocs/l3-l2-comm-data-path-refactor-impl.md
Comment on lines
+416
to
+420
| `SIGNAL_NOTIFY`: | ||
|
|
||
| - `Set`: store the operand as `int32_t`. | ||
| - `Add`: load current `int32_t`, add operand, store result. | ||
| - Apply release/publish ordering before the store becomes visible. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "NotifyOp\.Add|SIGNAL_NOTIFY|counter_operand|int32_t" python src tests docsRepository: hw-native-sys/simpler
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target section with line numbers
sed -n '400,435p' docs/l3-l2-comm-data-path-refactor-impl.md
# Find the upstream implementation/contract for SIGNAL_NOTIFY semantics
rg -n "SIGNAL_NOTIFY|NotifyOp\.Add|Add: load current|store the operand as int32_t|release/publish ordering|poison|exception" \
python src docs testsRepository: hw-native-sys/simpler
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- docs/l3-l2-comm-data-path-refactor-design.md (around NotifyOp.Add) ---'
sed -n '288,312p' docs/l3-l2-comm-data-path-refactor-design.md
echo '--- docs/l3-l2-orch-comm.md (counter notify semantics) ---'
sed -n '300,330p' docs/l3-l2-orch-comm.md
echo '--- python/simpler/l3_l2_orch_comm.py (SIGNAL_NOTIFY path) ---'
sed -n '200,250p' python/simpler/l3_l2_orch_comm.py
echo '--- src/common/platform/shared/host/l3_l2_orch_comm_service.cpp (SIGNAL_NOTIFY handler) ---'
sed -n '181,330p' src/common/platform/shared/host/l3_l2_orch_comm_service.cpp
echo '--- tests around NotifyOp.Add and counter behavior ---'
sed -n '280,340p' tests/ut/py/test_worker/test_l3_l2_orch_comm.py
sed -n '100,140p' tests/ut/py/test_worker/test_l3_l2_message_queue.pyRepository: hw-native-sys/simpler
Length of output: 15045
Define SIGNAL_NOTIFY.Add overflow behavior. The current parent implementation does observed + request.counter_operand on int32_t, so overflow is undefined unless you specify wrap, saturate, or error semantics here.
🤖 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 `@docs/l3-l2-comm-data-path-refactor-impl.md` around lines 416 - 420, Clarify
the SIGNAL_NOTIFY.Add overflow behavior in the comm data path spec, since the
current description only says to load, add, and store an int32_t result. Update
the SIGNAL_NOTIFY section to explicitly state whether the addition should wrap,
saturate, or fail on overflow, and make the wording consistent with the parent
implementation behavior so readers can implement the add operation
unambiguously.
ccyywwen
force-pushed
the
l3-l2-comm-data-path-refactor
branch
2 times, most recently
from
7ef64f9 to
0d0988f
Compare
- Template L3L2QueueEndpoint on MaxInflight and remove runtime config - Move active input tracking to a fixed ring with STOP extra capacity - Unify L3-L2 helper namespaces, error metadata, cache, and time helpers - Link public AICPU helper sources into JIT orchestration builds - Update docs, examples, and C++ UT coverage
- Add the direct parent data path design for L3-L2 orch comm - Add an implementation plan with lifecycle controls, sim/onboard migration split, cleanup ordering, and validation scope
- Add lifecycle create/release controls and fixed create wire structs - Implement sim L2 Host owned region allocation with L3 Host mapping helpers - Route sim payload and counter operations through mapped-region helpers - Keep queue staging behavior behind primitive payload transfer - Add focused Python and C++ coverage for lifecycle and mapped access
- Describe L3 Host mapped-region behavior for simulation platforms - Clarify onboard payload buffers and L2 Host-mediated operations - Move queue payload-transfer staging wording to the primitive backend
ccyywwen
force-pushed
the
l3-l2-comm-data-path-refactor
branch
from
0d0988f to
eccc8ef
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
2 participants
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
This PR is based on PR #1236 /
l3-l2-message-queue-input-window.That base is intentional: PR #1236 restructures the L3-L2 message queue and
primitive payload-transfer layering, so the mapped-region data path here is
implemented against that newer primitive-layer shape rather than the older queue
staging structure.
This PR implements the first production slice of the L3-L2 communication data
path refactor. It adds lifecycle create/release controls for L3-L2 regions and
switches simulation platforms to an L3 Host mapped-region data path. In sim,
payload and counter operations now execute from the L3 Host through private
mapped-region helpers, while the six-scalar descriptor remains unchanged and
continues to carry L2-side addresses for the L2 AICPU endpoint.
On onboard platforms, the public API remains supported through the existing
L2 Host-mediated path. No public primitive or message queue API changes are
introduced.
Changes
Python encode/decode support.
and counter ranges.
read/write, and counter notify/test/wait.
through
descriptor_scalars().payload_write,payload_read,SIGNAL_NOTIFY,SIGNAL_TEST,and
SIGNAL_WAITthrough the mapped-region path.cleanup ordering.