iron: sync freshly written arena buffers to the device in the fused ELF callable#138
Open
atassis wants to merge 1 commit into
Open
iron: sync freshly written arena buffers to the device in the fused ELF callable#138atassis wants to merge 1 commit into
atassis wants to merge 1 commit into
Conversation
…LF callable
SequenceFullELFCallable rewrites its input and scratch arenas on every dispatch
through get_buffer() sub-views, but writing a sub-view's .data did not mark the
parent XRTTensor host-dirty. The parent's residency flag stayed "npu" from
allocation, so _sync_inputs' input_buffer.to("npu") no-opped on the residency
guard and the freshly written inputs (and resident weights loaded via
scratch_buffer.to("npu")) never reached the device. The kernel then ran on the
zeros left by the one-time init sync, so the output was zero or, as unsynced host
bytes intermittently became visible across the aliased mapping, partial and
nondeterministic.
Fix at the two authoritative points:
- XRTSubBuffer.data marks the sub-view and its parent "cpu" on access, so a
later parent .to("npu") fires the host to device sync. numpy gives no write
hook and callers write with np.copyto (buffer protocol, not __setitem__), so
marking on access is the reliable choice; a redundant read sync is cheap, a
skipped write sync is a correctness bug.
- _sync_outputs asserts device residency before .to("cpu") so the device to
host sync fires every dispatch, including after a prior output read marked the
buffer "cpu" (otherwise a looped dispatch reads stale output).
Verified on AIE2P: a single GEMV reaches rel-L2 0.0017 and a weighted RMSNorm
0.0087 against the host reference, both reliable and deterministic across repeated
processes and looped dispatches, where before they were all zero, bimodal, or
rel-L2 near 1.0.
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
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.
iron: sync freshly written arena buffers to the device in the fused ELF callable
Summary
A fused-ELF
OperatorSequencedispatch silently runs on stale (init-zero) inputs, because writing aget_buffer()sub-view's.datanever marks the parent buffer host-dirty, so the per-dispatch host to device sync is skipped by a residency-flag optimization. The op computes on zeros and produces zero, or partial and nondeterministic, output. This fixes it in the two places that own the arena sync.Root cause
SequenceFullELFCallablere-authors its input and scratch arenas on every dispatch through the sub-views returned byget_buffer(). Those sub-views (XRTSubBuffer) alias the parentXRTTensor's buffer.XRTTensor.to(target)is a residency tracker with a skip optimization: it only performs the copy when the trackeddeviceflag differs from the target. The input arena is allocated withdevice="npu"and synced once at construction, so its flag is"npu". Writing a sub-view's.dataafterward does not change that flag, so_sync_inputs'input_buffer.to("npu")sees"npu" == "npu"and does nothing. The freshly written inputs, and resident weights that a caller loads withscratch_buffer.to("npu"), never reach the device. The kernel then runs on the zeros left by the one-time init sync.The failure looks intermittent rather than always-zero because the sub-view and parent alias the same pages, so freshly written host bytes sometimes become visible to the device without an explicit sync and sometimes do not. The existing
XRTSubBuffer.to()FIXME already flags this sub-buffer sync ambiguity and asks for per-region dirty tracking.Fix
Two authoritative points,
iron/common/utils.pyandiron/common/sequence.py:XRTSubBuffer.datamarks the sub-view and its parent"cpu"on access. A later parent.to("npu")then actually fires the host to device sync. numpy gives no write hook, and callers write withnp.copyto(sub.data, ...), which goes through the buffer protocol rather than__setitem__, so a precise write-only hook would miss exactly those writes. Marking on access is the reliable choice: a redundant read sync is cheap, a silently skipped write sync is a correctness bug._sync_outputsasserts device residency before.to("cpu")._runjust rewrote the output arena on the device, so the device holds the authoritative copy; forcing the flag makes the device to host sync fire every dispatch, including after a prior read of the output.datamarked the buffer"cpu". Without this a looped dispatch would read stale output.19 lines across 2 files, no API change.
Alternative considered
Forcing the sync at the two sync sites only (
_sync_inputshost to device,_sync_outputsdevice to host) and leaving.datauntouched is cleaner for the input and output arenas, but it does not cover resident weights, which callers load into the scratch arena and sync withscratch_buffer.to("npu"); that user-driven.to()still needs the write to have marked the buffer dirty. Marking on.dataaccess is the smaller change that makes the existingget_buffer().dataplus.to()pattern correct for all three arenas. Happy to switch to whichever the maintainers prefer, or to a proper per-region dirty flag as the FIXME suggests.Verification
On AIE2P / NPU2, against a host reference, before and after:
End to end, a full 18-layer Gemma-3-270M greedy decode ("The capital of France is") went from 0 of 8 tokens matching the host oracle (garbage first token) to generating the correct answer " Paris" and the first 3 tokens exactly. The remaining decode divergence is a separate numerical matter, not this bug.
Repro
Self contained, no external weights. Prints
rel-L2and asserts correctness; fails on the currentdevel, passes with this change:Runs on any AIE2P device with a single dispatch. I can fold this into the test suite in whatever form you prefer.