Fix: correct documentation that contradicts the code - #1534
Merged
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDocumentation and inline comments were updated to match current SDMA provisioning, kernel-launch caching, worker completion polling, communication backends, ABI details, platform behavior, and review-document references. ChangesDocumentation alignment
Estimated code review effort: 2 (Simple) | ~10 minutes 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: 2
🤖 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/worker-manager.md`:
- Around line 174-180: Update the comments describing next_liveness_check and
the related liveness polling logic to call the timing source a “steady clock” or
“monotonic clock” instead of a “wall clock,” matching steady_clock::now()
behavior; leave the implementation unchanged.
- Around line 176-183: Update the pseudocode loop around next_liveness_check to
invoke check_child_death() when the poll interval expires, and return the
endpoint failure or otherwise terminate the wait when the child is detected
dead. Preserve the existing TASK_DONE polling and next-check scheduling behavior
so the example reflects the WorkerManager liveness logic.
🪄 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: 8fb41aae-dac4-4a8b-b340-05f85ed442c2
📒 Files selected for processing (22)
docs/a5-sdma-overlay.mddocs/aicpu-kernel-launch-mechanisms.mddocs/comm-domain.mddocs/dynamic-linking.mddocs/hardware/chip-architecture.mddocs/investigations/2026-07-a2a3-sdma-fault-teardown.mddocs/worker-manager.mdsrc/a2a3/platform/onboard/host/CMakeLists.txtsrc/a2a3/platform/onboard/host/comm_hccl.cppsrc/a2a3/platform/sim/host/CMakeLists.txtsrc/a2a3/runtime/host_build_graph/runtime/backend/sdma/sdma_completion_kernel.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/backend/sdma/sdma_completion_kernel.hsrc/a5/platform/onboard/host/CMakeLists.txtsrc/a5/platform/onboard/host/comm_hccl.cppsrc/a5/platform/sim/host/CMakeLists.txtsrc/a5/runtime/tensormap_and_ringbuffer/runtime/backend/sdma/sdma_completion_kernel.hsrc/common/aicpu_loader/README.mdsrc/common/platform/onboard/host/device_runner_helpers.hsrc/common/platform/sim/host/device_runner_base.hsrc/common/platform_comm/comm_context.hsrc/common/worker/pto_runtime_c_api.htests/ut/cpp/a5/test_aicore_completion_mailbox.cpp
💤 Files with no reviewable changes (2)
- src/a5/platform/onboard/host/CMakeLists.txt
- src/a2a3/platform/onboard/host/CMakeLists.txt
ChaoWao
force-pushed
the
fix/stale-doc-claims
branch
2 times, most recently
from
July 28, 2026 00:46
9d6ca86 to
157162d
Compare
Compiling the capability survey turned up documentation and comments that were true once and now disagree with what the code does. Each was re-verified against the current source before being changed here; several were load-bearing enough to send a reader the wrong way. Claims corrected: - docs/hardware/chip-architecture.md called the AIC_CTRL MMIO window Device-nGnRnE. It is nGnRE, proven from driver source in docs/hardware/mmio-performance.md, and the distinction is what makes the STR/LDR asymmetry there possible. - docs/worker-manager.md showed the dispatch wait as sleep_for(50us) and called it "not busy-wait". The code spin-polls and cites codestyle rule 5 for why it must, sampling child liveness on a steady-clock period. Its pseudocode also dropped the check_child_death() call that ends the wait with ENDPOINT_FAILURE, so as written the loop would have spun forever on a dead child. - docs/dynamic-linking.md claimed the AICore ELF is registered on every launch_aicore_kernel() call; registration is lazy and the handle cached. Its flow diagram also had the launch order backwards: AICore launches before the AICPU Run task, via rtKernelLaunchWithHandleV2, with no intervening stream sync. - docs/comm-domain.md described the window as VMM plus shareable-handle import. a2a3 prefers Fabric V2 handles and falls back to VMM IPC; a5 uses VMM shareable handles only. Peer access via aclrtDeviceEnablePeerAccess is still accurate and stays. - docs/comm-domain.md said a producer CoreCallable declares the SDMA workspace requirement. No such API exists; the demo provisions SDMA with Worker(enable_sdma=True). - docs/a5-sdma-overlay.md called the a2a3 SDMA path "always on". The provider is always compiled; provisioning is opt-in and defaults off. - The a2a3 SDMA fault-teardown investigation called sdma_async_completion_demo unaffected by that change, but the demo sets enable_sdma=True and so carries the same teardown cost. - src/common/worker/pto_runtime_c_api.h and the sim device_runner_base.h both documented a CallConfig block_dim knob. CallConfig has no such field and says so explicitly. - src/common/aicpu_loader/README.md resolved one symbol rather than every declared one, put device_id in the per-task KernelArgs when it reaches the device once via InitArgs, and gave a per-runtime dispatcher path when the SO is shared per arch under build/lib/<arch>/dispatcher/. - A cpput comment claimed a5 is counter-only; a5 registers SDMA event-record and URMA event-handle backends too. - docs/aicpu-kernel-launch-mechanisms.md credited PR hw-native-sys#537 with attempting Path B. It is merged and shipped Path A. Also removes seven references to files under .docs/, which is gitignored and absent from every checkout, so no reader could follow them. The surrounding comments now state the invariant that survives on its own rather than recording where a decision was taken: comments.md keeps change-rationale and edit history out of code, so a pull-request number is no more appropriate there than the unresolvable path was. The two remaining .docs/ mentions are left alone: docs/ci.md lists it as a NON_CODE path set member, and the review-pr skill creates .docs/review/ as its own scratch directory. Comments and prose only — no statement or expression changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai Both inline findings addressed and resolved:
The review body only enumerated those two threads, so no separate action; the walkthrough comment needs none. Both fixes are folded into the existing single commit rather than appended. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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
Compiling the capability survey (#1525) surfaced documentation and comments that were true once and now disagree with the code. Every claim was re-verified against current
mainbefore being changed here — one item the survey reported turned out to be wrong and is not included (see below).Comments and prose only: the diff contains no statement or expression changes, verified by filtering the
.h/.cpp/CMakeLists.txtdiff for non-comment lines.What was wrong
docs/hardware/chip-architecture.mdDevice-nGnRnEnGnRE, proven from driver source inmmio-performance.md— the distinction is what permits the STR/LDR asymmetry documented theredocs/worker-manager.md(×2)sleep_for(50us), "not busy-wait"docs/dynamic-linking.mdlaunch_aicore_kernel()device_runner_base.cpp:1101)docs/dynamic-linking.mdrtKernelLaunchrtKernelLaunchWithHandleV2, no intervening syncdocs/comm-domain.mddocs/comm-domain.mdCoreCallabledeclares the SDMA workspaceWorker(enable_sdma=True)docs/a5-sdma-overlay.mdinvestigations/2026-07-a2a3-sdma-fault-teardown.mdsdma_async_completion_demounaffectedenable_sdma=True, so it carries the same teardown costpto_runtime_c_api.h, simdevice_runner_base.hCallConfigcarries ablock_dimknobcall_config.h:22— "There is no block_dim knob"aicpu_loader/README.md(×3)device_idin per-taskKernelArgs; per-runtime dispatcher pathdevice_idarrives once viaInitArgs; dispatcher is shared per arch underbuild/lib/<arch>/dispatcher/tests/ut/cpp/a5/test_aicore_completion_mailbox.cppdocs/aicpu-kernel-launch-mechanisms.mdAlso: seven dangling
.docs/references.docs/is gitignored (.gitignore:31) and absent from every checkout, so seven comments pointed at documents no reader could open. The surrounding comments now state the invariant that survives on its own, rather than recording where a decision was taken —comments.mdkeeps change-rationale and edit history out of code, so a pull-request number is no more appropriate there than the unresolvable path was. Concretely,comm_context.hnow states that its layout must stay byte-equivalent with pto-isa's parallelHcclDeviceContextdeclaration, instead of narrating which paths were deleted and where that was discussed.PR numbers are kept in
docs/aicpu-kernel-launch-mechanisms.md:comments.mdnames docs as a legitimate home for change-context, and that page is the forensic record of why Path B is unusable, so the issue and PR numbers are its evidence.Two
.docs/mentions are deliberately kept:docs/ci.mdlists it as a member of the CI'sNON_CODEpath set, and thereview-prskill creates.docs/review/as its own scratch directory. Both are correct usages.One survey claim I did not act on
The survey reported that
docs/troubleshooting/a2a3-507899-aicpu-shared-so-fault.mdrepeats thedevice_id-in-KernelArgserror. On inspection it does not — it refers toDeviceArgs.device_idand the inner-SO filename, both accurate. Left unchanged.I also left the doc's
Path A/Path Blabels alone. The survey called them inverted relative to issue #822's wording, but the doc is internally consistent and its technical content matches the code (Path B =KERNEL_TYPE_AICPU_CUSTOM, absent fromsrc/, correctly marked broken). Renaming to match an issue's informal phrasing would be churn.Testing
CMakeLists.txt; brace/quote balance on the modified cpput filemarkdownlint-cli2clean on all 8 changed markdown files (v0.20.0, the version.pre-commit-config.yaml:83pins).h/.cpp/CMakeLists.txtso the full CI runs rather than the docs-only path