Repository navigation
Docs: separate what the descriptor's visibility chain proves from what it assumes - #2368
Merged
ChaoWao merged 1 commit intoSep 19, 2026
Conversation
…t it assumes hw-native-sys#2254's field inventory observed that three onboard variants invalidate the whole device descriptor at end of run while a5 tensormap_and_ringbuffer does not, and read that as a missing operation. Tracing the chain — host snapshot, slot-owned destination, synchronous H2D, launch, first device read — at 31da056 does not support that reading, and does not support the opposite one either. The documented rule in docs/hardware/cache-coherency.md says a5 host-DMA writes are coherent with AICPU reads and that a5 must therefore not invalidate. Against that rule a5 tensormap_and_ringbuffer is the conforming variant and a5 host_build_graph is the departure, calling the operation at two sites. The rule also disagrees with the code in placement and extent: it reads "before reading host-written Runtime" while all four call sites sit in deinit() at the end of the previous run, and its snippet's sizeof(Runtime) predates the narrowing to the descriptor. The invariant neither document states is that the earliest descriptor reader is the platform AICPU kernel's affinity gate, not the runtime executor. That gate performs no cache operation, so on every variant the first read of host-published bytes happens with no maintenance earlier in the same invocation, and because the gate decides how many threads proceed a stale read changes the thread population rather than a value. Four assumptions are recorded as unavailable rather than argued against: an end-of-run invalidate creates no ordering against a later host DMA, a slot's first run has no previous deinit so no invalidate ever ran for those lines, invalidation is not an acquire, and cache_invalidate_range is dc civac, which also writes AICPU-dirtied descriptor bytes back. Separately established and previously unwritten: a5 host_build_graph's 64-byte Handshake line has three whole-line writers — host, AICore CACHELINE_OUT and AICPU dc cvac — whose write-backs cannot merge per word, and aicore_done is what serializes them, published by the same flush that would otherwise overwrite the host's words. No operation is changed and no fix is proposed, because the candidates are mutually exclusive and each depends on one of three unknowns this repository cannot settle: whether a5's coherency claim holds, whether kernel launch leaves the AICPU cache cold, and whether GM is cacheable for the AICPU at all. A probe specification for each is included with its positive control and with what measurement cannot establish; none was run. Relates to hw-native-sys#2254. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
2 tasks done
Collaborator
Author
|
Static review of this PR landed after it merged. Several conclusions here promoted source control flow, or observations that do not discriminate, into hardware guarantees, and are withdrawn in #2369 — the marker-serialization claim, "the affinity gate is the earliest device read", the kernel-mode repeated-run contract, write-through as an answer to input freshness, and the probe plan's stated classifications. The source did not change: same baseline |
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Sep 19, 2026
Static review of hw-native-sys#2368 found that several conclusions promoted source control flow, or observations that do not discriminate, into hardware guarantees. The source has not changed and the baseline SHA is the same; only the claims do. Each retraction is marked in place rather than deleted, so a reader arriving from hw-native-sys#2368 sees which claim was withdrawn and why. Four corrections. The report gate is a dependency the code expresses, not proven serialization. The first version said aicore_done "serializes" a5 host_build_graph resident's three handshake writers and "keeps it correct". Observing the marker is not an observation that the producer's whole-line write-back retired — the post-dcci dsb orders that producer's own later accesses and nothing more — and it does not establish that the AICPU's clean-and-invalidate cannot write back an older copy. Those are the same publication and ownership assumptions the entry leaves open elsewhere, so the dependency cannot be promoted using the document's own unknowns. No currently executing race is established either. The discussion is now scoped to resident, whose function ends at write_reg(COND, AICORE_EXITED_VALUE) with no final handshake dcci, and each path's last descriptor access is listed separately instead of borrowing the legacy path's exit flush. The first AICPU read is not the first device read. Both launch paths submit AICore before AICPU on distinct streams (device_runner.cpp:664-687), so no chronological claim spans the two processors. First reads are now given per processor and mode. The a5 host_build_graph AICore reads the host-published mode word at aicore_executor.cpp:533-535 and consumes the host context address and its pointee before READY, ahead of any AICPU permission; the other paths' first AICore descriptor access is their own report write, with the task pointer read after window-open. Kernel mode is scaffolding and its device-read chain is not delivered. prepare_once provisions storage, but simpler_kernel_mode_supported returns 0 and simpler_kernel_mode_launch validates arguments then unconditionally returns INVALID_STATE (c_api_shared.cpp:1561, :1700). The inferred repeated-run cache contract is withdrawn. Cache-policy alternatives do not imply the proposed removals. Write-through is removed as a sufficient answer to input freshness: it governs writes, while a write-through cache still holds readable lines that external DMA can leave stale. Because cache_invalidate_range is clean-and-invalidate it may serve both an inbound freshness role and an outbound publication role for device-authored fields, so no single unknown licenses deleting it — host-DMA coherence answers one input edge, and a5 not being coherent still would not make a5 tensormap_and_ringbuffer defective while launch maintenance and mapping remain open. The "mutually exclusive candidate fixes" list is replaced by the individual obligations any change must discharge. Same-slot reuse is described by what the code proves — try_reserve_native_run serializes preparations on a slot, and drain and retirement precede the next preparation — rather than by calling the ordering probable. The probe specification is demoted to open questions. A cross-arch run is not a positive control until a stale readable line is independently established; a dirty-sentinel run cannot separate a surviving cache from an old line written back over the host value and then read fresh, nor either from plain eviction; and page attributes are not classifiable by timing against an MMIO reference, so the "flat ratio indicates uncached" rule is withdrawn. Each is now stated with its non-unique interpretations and the verified initial state it would need. Also corrected: a fresh allocation is new ownership, not proof of new physical backing or a cold cache; observed stores are distinguished from an established dirty write-back policy; and CI passing is recorded as an observation of the exercised cases rather than a frequency bound. The stale statements in docs/hardware are corrected only where directly verified against the source — the call's location in deinit() and its sizeof(runtime->dev) extent, plus a note that a5 host_build_graph calls the operation while a5 tensormap_and_ringbuffer does not. The a5 coherency claim itself is left as it stands, since settling it needs evidence this work does not have. Relates to hw-native-sys#2254. Corrects hw-native-sys#2368. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Sep 19, 2026
…#2369) Static review of #2368 found that several conclusions promoted source control flow, or observations that do not discriminate, into hardware guarantees. The source has not changed and the baseline SHA is the same; only the claims do. Each retraction is marked in place rather than deleted, so a reader arriving from #2368 sees which claim was withdrawn and why. Four corrections. The report gate is a dependency the code expresses, not proven serialization. The first version said aicore_done "serializes" a5 host_build_graph resident's three handshake writers and "keeps it correct". Observing the marker is not an observation that the producer's whole-line write-back retired — the post-dcci dsb orders that producer's own later accesses and nothing more — and it does not establish that the AICPU's clean-and-invalidate cannot write back an older copy. Those are the same publication and ownership assumptions the entry leaves open elsewhere, so the dependency cannot be promoted using the document's own unknowns. No currently executing race is established either. The discussion is now scoped to resident, whose function ends at write_reg(COND, AICORE_EXITED_VALUE) with no final handshake dcci, and each path's last descriptor access is listed separately instead of borrowing the legacy path's exit flush. The first AICPU read is not the first device read. Both launch paths submit AICore before AICPU on distinct streams (device_runner.cpp:664-687), so no chronological claim spans the two processors. First reads are now given per processor and mode. The a5 host_build_graph AICore reads the host-published mode word at aicore_executor.cpp:533-535 and consumes the host context address and its pointee before READY, ahead of any AICPU permission; the other paths' first AICore descriptor access is their own report write, with the task pointer read after window-open. Kernel mode is scaffolding and its device-read chain is not delivered. prepare_once provisions storage, but simpler_kernel_mode_supported returns 0 and simpler_kernel_mode_launch validates arguments then unconditionally returns INVALID_STATE (c_api_shared.cpp:1561, :1700). The inferred repeated-run cache contract is withdrawn. Cache-policy alternatives do not imply the proposed removals. Write-through is removed as a sufficient answer to input freshness: it governs writes, while a write-through cache still holds readable lines that external DMA can leave stale. Because cache_invalidate_range is clean-and-invalidate it may serve both an inbound freshness role and an outbound publication role for device-authored fields, so no single unknown licenses deleting it — host-DMA coherence answers one input edge, and a5 not being coherent still would not make a5 tensormap_and_ringbuffer defective while launch maintenance and mapping remain open. The "mutually exclusive candidate fixes" list is replaced by the individual obligations any change must discharge. Same-slot reuse is described by what the code proves — try_reserve_native_run serializes preparations on a slot, and drain and retirement precede the next preparation — rather than by calling the ordering probable. The probe specification is demoted to open questions. A cross-arch run is not a positive control until a stale readable line is independently established; a dirty-sentinel run cannot separate a surviving cache from an old line written back over the host value and then read fresh, nor either from plain eviction; and page attributes are not classifiable by timing against an MMIO reference, so the "flat ratio indicates uncached" rule is withdrawn. Each is now stated with its non-unique interpretations and the verified initial state it would need. Also corrected: a fresh allocation is new ownership, not proof of new physical backing or a cold cache; observed stores are distinguished from an established dirty write-back policy; and CI passing is recorded as an observation of the exercised cases rather than a frequency bound. The stale statements in docs/hardware are corrected only where directly verified against the source — the call's location in deinit() and its sizeof(runtime->dev) extent, plus a note that a5 host_build_graph calls the operation while a5 tensormap_and_ringbuffer does not. The a5 coherency claim itself is left as it stands, since settling it needs evidence this work does not have. Relates to #2254. Corrects #2368.
This was referenced Sep 19, 2026
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
#2254's field inventory observed that three onboard variants invalidate the whole device descriptor at end of run while a5
tensormap_and_ringbufferdoes not, and read that as a missing operation. This entry traces the chain — host snapshot → slot-owned destination → synchronous H2D → launch ordering → first device read → last access → reuse — at a pinned SHA, and finds the original reading unsupported. It does not support the opposite reading either.Docs only. No cache operation is changed, no contract is created, nothing was built, run or measured.
What the trace found
The asymmetry inverts once the documented rule is applied.
docs/hardware/cache-coherency.md:130-145states that a5 host-DMA writes are coherent with AICPU reads and that a5 therefore does not invalidate. Against that rule a5tensormap_and_ringbufferis the conforming variant, and a5host_build_graphis the departure — it calls the operation at two sites.The rule also disagrees with the code in two further ways. It is written as "before reading host-written Runtime", and
chip-architecture.md:218-219places it at AICPU entry; all four call sites are indeinit(), at the end of the previous run. And its snippet'ssizeof(Runtime)extent predates #2309/#2315's narrowing tosizeof(dev).The invariant neither document states: the earliest descriptor reader is the platform AICPU kernel's affinity gate (
kernel.cpp:93/:121), not the runtime executor. That gate performs no cache operation, so on every variant the first read of host-published bytes happens with no maintenance earlier in the same invocation — and because the gate decides how many threads proceed, a stale read changes the thread population rather than a value.Deliverable matrix covers a2a3/a5 × TRB/HBG, with a5 HBG resident and legacy listed separately (the resident/legacy branch is itself taken by reading the host-published mode word) and program versus kernel
prepare_once, where the single upload per context makes the visibility question different in kind rather than a variant of the program-mode one.Four assumptions recorded as unavailable
Not argued against — recorded as things the code does not establish:
dsb sy; isbcompletes the maintenance and nothing more.deinit, so no invalidate has ever executed for those lines.cache_invalidate_rangeisdc civac— clean and invalidate — so atdeinitit also writes AICPU-dirtied descriptor bytes back, the opposite direction from the one its comment implies. It is also line-granular and rounds outward.Separately established, and previously unwritten
a5 HBG's 64-byte
Handshakeline has three whole-line writers: the host (aicpu_readymode +taskcontext address), the AICore (dcci … CACHELINE_OUT) and the AICPU (dc cvac). Whole-line write-backs cannot merge at word granularity, so ordering is the only thing that makes this safe — and what provides it isaicore_done, published by the very flush that would otherwise overwrite the host's words. The AICPU does not reachpublish_context_partitionuntil it has observed every core'saicore_done.That ordering is load-bearing and documented nowhere. It is what a future change to this line would silently break, and it is documentable today because it is a property of the current code rather than of the hardware.
Why no fix is proposed
The candidates are mutually exclusive, and which is right depends on three unknowns this repository cannot settle:
Device-nGnREattribute covers the MMIO window only.Any one being true would make some or all four call sites redundant. Shipping a change now would encode a guess as an invariant.
A probe specification for each unknown is included, with its positive control, the version/binary provenance to record, and an explicit statement of what measurement cannot do — a probe can demonstrate a violation but cannot produce an architectural guarantee. None was run, and sim cannot exercise any of it, since both cache primitives are no-ops there.
Baseline and evidence limits
Every anchor read at
31da0560e202b7bb378773f7dfc1d0074fb28e89(main, the merge of #2363), pinned in the document. No hardware number from any other commit is carried forward. All cited line anchors were verified against that tree.Three documentation corrections are identified but deliberately not made, because each depends on an unknown — correcting them now would replace one unsourced claim with another. Three decisions are returned to the maintainer rather than taken.
Scope
Does not touch the descriptor gate-tail relocation or the full-allocation/prefix-upload separation in flight elsewhere, nor E1 health or DFX export. Read-only analysis of published
main.Testing
markdownlint-cli2with the repo config — 0 errors on both filesRelates to #2254.
🤖 Generated with Claude Code