[AIR] Replace the memtile fan-in/fan-out daisy chain with a per-slot rendezvous - #2002
Draft
erwei-xilinx wants to merge 2 commits into
Draft
erwei-xilinx wants to merge 2 commits into
erwei-xilinx wants to merge 2 commits into
Conversation
…rendezvous gemma4-e2b's PLE decode hangs in firmware TDR on an NPU2 Krackan -- 7 of 20 dispatches at default power mode, ERT_CMD_STATE_TIMEOUT and ERT_CMD_STATE_ABORT at the 7.0/9.0 s watchdog. This is the second defect behind issue #1984, the one that survives the DECODE_STACK fix. It is a cyclic wait between AIR's serialized fan-in chain lock and a shared switchbox arbiter. buf143/buf162 on mem_tile_2_1 gather the four projection columns: a chain-lock fan-in set, 4 writers, 2 ping-pong slots. pickChainBdLocks daisy-chained them, cap -> W0 -> W1 -> W2 -> W3 -> R -> cap, which imposes a compile-time total order on arrivals that are independent at runtime -- four free-running herds in four columns. Nothing makes the two orders agree. All four gather streams egress to the shim row and travel west. Switchbox (2,0) has 9 masters and 6 arbiters, so three pairs must share, and the pathfinder put W0 and W2 on arbiter 1 (W1 and W3 on arbiter 2). A master port is backpressured until its S2MM channel's lock frees, and an arbiter granted to a stalled packet cannot grant another slave. So when ping-pong lets the east columns run a round ahead, W2's packet wins arbiter 1, stalls waiting for W1's chain token, and holds the arbiter W0 needs -- W0 being the writer the whole chain is waiting on. Circular wait. That predicts the per-herd bisect exactly. Denying ping-pong, (blk0, blk1) = (no, yes) 0/20, (yes, yes) 4/20, (yes, no) 20/20, (no, no) 19/20. Only run-ahead of the LATE stages matters, because an earlier stage holding the arbiter cannot close a cycle: the later stage was waiting on it anyway. The PLE is the amplifier, not the cause -- it lengthens the reader's credit-return path so the stall window is wide enough to overtake. So give each buffer SLOT its own (capacity, signal) pair instead of chaining the stages. For slot s the multi side acquires cap[s] by 1 and releases sig[s] by 1; the single side acquires sig[s] by N and releases cap[s] by N, with cap[s] primed to N. Each slot is a counted barrier and the N participants are mutually unordered, so an early arrival costs latency and can never hold a resource an earlier stage needs. The cycle is structurally impossible however the pathfinder assigns amsels. Run-ahead stays bounded to pp_slots: the (pp_slots+1)'th transfer wraps to slot 0, whose capacity the single side has not replenished. Indexing by slot is what makes a counted lock work here, and is why the daisy chain looked necessary. One shared counting semaphore cannot bound per-participant run-ahead -- a single writer takes every credit and laps the reader -- and an AIE2 BD has exactly one lock-acquire and one lock-release field, so an N-way join is not expressible either. Per-slot counted locks need neither. Refeed buffers keep the daisy chain, behind ChainLockSet::serialized. Their credits come from air.refeed_count rather than the participant count and they never get a ping-pong twin, so the skew cannot arise; the refeed test passes untouched. air.no_chain_lock stays honored for fan-out only, so designs carrying it keep their lowering -- with the participants no longer serialized it is now a no-op in practice, but silently changing what a tagged buffer lowers to is not this commit's business. Measured on device at default power mode, same tree and geometry apart from the lock template: daisy chain 7/20, per-slot rendezvous 40/40, both emitting the same logits. No performance change -- the control's non-hanging dispatches run 64.5-64.6 ms against the fix's 65.1 ms mean. Four tests pinned the old chain and now assert the new invariant, that every participant takes the SAME lock for a given slot. memtile_chain_lock_v2_fanin_n3.mlir is new: it pins the credit count to the participant count rather than the constant 4, and covers an odd N. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The rendezvous protocol has an unresolved correctness concern, and several tests do not enforce distinct locks for each slot.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Replaces memtile fan-in/fan-out daisy-chain locks with per-slot counted rendezvous locks.
Changes:
- Adds per-slot capacity/signal locks and ping-pong handling.
- Updates DMA lock selection and counts.
- Refreshes fan-in, fan-out, opt-out, and odd-participant tests.
| File | Reviewed change | Review status |
|---|---|---|
mlir/test/Conversion/AIRToAIE/memtile_no_chain_lock_fanin_ignored.mlir |
Updates fan-in opt-out expectations. | Reviewed |
mlir/test/Conversion/AIRToAIE/memtile_chain_lock_v2_pingpong.mlir |
Tests slot-specific ping-pong locks. | Add explicit slot-lock distinctness assertion. |
mlir/test/Conversion/AIRToAIE/memtile_chain_lock_v2_fanout.mlir |
Tests fan-out rendezvous behavior. | Add explicit slot-lock distinctness assertion. |
mlir/test/Conversion/AIRToAIE/memtile_chain_lock_v2_fanin.mlir |
Tests fan-in rendezvous behavior. | Add explicit slot-lock distinctness assertion. |
mlir/test/Conversion/AIRToAIE/memtile_chain_lock_v2_fanin_n3.mlir |
Covers odd participant counts. | Reviewed |
mlir/lib/Conversion/AIRToAIESchedulingUtils.cpp |
Implements rendezvous lock allocation and selection. | Critical concern: aggregate counting may allow repeated participation by one writer before others arrive, risking stale data. |
mlir/lib/Conversion/AIRToAIEPass.cpp |
Applies per-BD counts and ping-pong slot locks. | Reviewed |
mlir/include/air/Conversion/AIRToAIESchedulingUtils.h |
Defines the per-slot lock-set model and APIs. | Reviewed |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+701
to
+703
| cls.cap_locks.push_back( | ||
| allocateLockOp(device, tile, /*init=*/cls.multiplicity())); | ||
| cls.sig_locks.push_back(allocateLockOp(device, tile, /*init=*/0)); |
erwei-xilinx
marked this pull request as draft
September 19, 2026 03:41
Four reflows reviewdog flagged, no semantic change. My local check had run on the wrong branch, so the CI failure was real. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The second defect behind #1984 — the one that survives the
DECODE_STACKfix in #1998. gemma4-e2b's PLE decode hangs in firmware TDR on NPU2 Krackan: 7 of 20 dispatches atdefaultpower mode,ERT_CMD_STATE_TIMEOUTandERT_CMD_STATE_ABORTat the 7.0/9.0 s watchdog.Cause
A cyclic wait between AIR's serialized fan-in chain lock and a shared switchbox arbiter.
buf143/buf162onmem_tile_2_1gather the four projection columns: a chain-lock fan-in set, 4 writers, 2 ping-pong slots.pickChainBdLocksdaisy-chained them,cap → W0 → W1 → W2 → W3 → R → cap, imposing a compile-time total order on arrivals that are independent at runtime — four free-running herds in four columns.All four gather streams egress to the shim row and travel west. Switchbox (2,0) has 9 masters and 6 arbiters, so three pairs must share, and the pathfinder put W0 and W2 on arbiter 1 (W1 and W3 on arbiter 2). A master port is backpressured until its S2MM channel's lock frees, and an arbiter granted to a stalled packet cannot grant another slave. So when ping-pong lets the east columns run a round ahead, W2's packet wins arbiter 1, stalls on W1's chain token, and holds the arbiter W0 needs — W0 being the writer the whole chain is waiting on.
It predicts the per-herd bisect cell by cell:
Only run-ahead of the late stages matters, because an earlier stage holding the arbiter cannot close a cycle — the later stage was waiting on it anyway. The PLE is the amplifier, not the cause: it lengthens the reader's credit-return path so the stall window is wide enough to overtake.
Fix
Each buffer slot gets its own (capacity, signal) pair instead of chaining the stages. For slot
s, the multi side acquirescap[s]by 1 and releasessig[s]by 1; the single side acquiressig[s]by N and releasescap[s]by N, withcap[s]primed to N. Each slot is a counted barrier and the N participants are mutually unordered, so an early arrival costs latency and can never hold a resource an earlier stage needs. Run-ahead stays bounded topp_slots.Indexing by slot is what makes a counted lock work here, and is why the daisy chain looked necessary: one shared counting semaphore cannot bound per-participant run-ahead, and an AIE2 BD has exactly one lock-acquire and one lock-release field, so an N-way join is not expressible either. Per-slot counted locks need neither.
Scope
ChainLockSet::serialized). Their credits come fromair.refeed_count, not the participant count, and they never get a ping-pong twin.memtile_chain_lock_v2_refeed.mlirpasses untouched.air.no_chain_lockstays honored for fan-out only, so tagged designs keep their lowering. It is now a no-op in practice, but changing that isn't this PR's business.use_lock_race_condition_fix_v2defaults off; onlyfused_decodeandfused_decode_pleenable it.Validation
On device at
defaultpower mode, same tree and geometry apart from the lock template:Both emit identical logits. No performance change — the control's non-hanging dispatches run 64.5–64.6 ms against the fix's 65.1 ms mean.
check-air-mlir557 tests, 539 passed, 0 failed.check-air-cpp1/1. clang-format 17 clean.fused_decode, the other v2 user, compiles clean.Four tests pinned the old chain and now assert the new invariant — every participant takes the same lock for a given slot.
memtile_chain_lock_v2_fanin_n3.mliris new: it pins the credit count to the participant count rather than the constant 4, and covers an odd N.Follow-up
run_npu2_sweep.lithas all eight contexts in--expect-failandrun_npu2_profile.litskips decode, both marked temporary. Flipping them needs #1998, #2000 and this all landed, so it's a separate PR.🤖 Generated with Claude Code