[AIEX] Add aiex.core_reset runtime-sequence op#3375
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new aiex.core_reset runtime-sequence operation to reset an AIE compute core via a masked pulse on the tile’s CORE_CONTROL.RESET bit, plus the corresponding lowering pass and lit coverage.
Changes:
- Introduce
aiex.core_resetop (with verifier restricting use to in-range core tiles on AIE2-family devices). - Add
--aie-lower-core-resetpass that lowersaiex.core_resetto twoaiex.npu.maskwrite32ops (assert + deassert) at tile-local offset0x32000, masked to bit 1. - Add lit tests for both lowering behavior and verifier diagnostics; wire the pass into the aiecc NPU lowering pipeline.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/aiecc/IRTransforms.h | Adds the new lowering pass into the NPU lowering pipeline. |
| lib/Dialect/AIEX/Transforms/CMakeLists.txt | Builds the new lowering implementation file. |
| lib/Dialect/AIEX/Transforms/AIELowerCoreReset.cpp | Implements conversion pattern + pass to lower aiex.core_reset to masked control writes. |
| lib/Dialect/AIEX/IR/AIEXDialect.cpp | Adds CoreResetOp verifier (AIE1 rejection, bounds checking, core-tile-only constraint). |
| include/aie/Dialect/AIEX/Transforms/AIEXPasses.td | Declares the new lowering pass and its registration metadata. |
| include/aie/Dialect/AIEX/Transforms/AIEXPasses.h | Declares the new pass factory function (and nearby pass factory declarations). |
| include/aie/Dialect/AIEX/IR/AIEX.td | Defines the new aiex.core_reset op, syntax, and documentation. |
| test/Passes/lower-core-reset/core_reset.mlir | Checks lowering emits the correct masked reset pulse and no unmasked write32. |
| test/Passes/lower-core-reset/core_reset_invalid.mlir | Checks verifier rejects mem/shim, out-of-range tiles, and AIE1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi, @jgmelber! Thanks for reviewing my work, again! I have a PR and idea, on which you might be a natural reviewer (this pr). I have added a |
|
@atassis I'd be interested in hearing more about your proposal but I have some trouble with the link. Can you please summarize here (or in a discussion)? |
|
https://atassis.github.io/writing/amd-contract-first-e99255/ does this help? Temporarily deployed it onto github pages. |
|
#3390 |
andrej
left a comment
andrej
left a comment
There was a problem hiding this comment.
It's useful to have an explicit instruction for core resets, thanks for this contribution.
But could we use an SSA tile, like in the channel reset op, rather than hard-coded column/row values?
| ins I32Attr:$column, | ||
| I32Attr:$row |
There was a problem hiding this comment.
Why not a reference to an SSA %tile?
There was a problem hiding this comment.
Good eye, I have missed that, thanks! Already fixed
Add a runtime-sequence op that resets a compute core, lowering to a masked pulse of the CORE_CONTROL reset bit (npu.maskwrite32). It mirrors aiex.set_lock: a non-blocking control op with no synchronization guarantees, meant to sit inside an aie.runtime_sequence. The runtime sequence can already set a lock (aiex.set_lock) but has no way to reset a compute core. The intended use is re-arming a resident kernel across dispatches without a full PDI reload: discard stale core state while keeping resident data in place. The lowering emits the reset as two npu.maskwrite32 writes masked to the reset bit (bit 1): assert, then deassert. CORE_CONTROL packs ENABLE (bit 0) and RESET (bit 1) in one word, so a plain write32 pulse would clobber ENABLE; masking to bit 1 preserves it. This mirrors aie-rt's XAie_CoreReset and XAie_CoreUnreset, which drive the reset bit with a MaskWrite32. The reset clears the core's processor state (PC and registers), not the tile's data memory, locks, or DMA; re-arming a core that has cleared ENABLE also needs a re-enable, which this op does not emit. The verifier accepts only core tiles: it rejects mem and shim tiles (no core to reset, matching XAie_CoreReset), coordinates outside the device, and AIE1 (the NPU runtime sequence has no meaning there, as SetLockOp also rejects).
AIEXPasses.h declared createAIELowerSetLockPass() twice. The second is redundant; removing it, per review.
Fix the code-format CI check: trim the file-header banner to 80 columns and let clang-format reflow a comment and a call.
Address review feedback: name the target tile with an SSA aie.tile value instead of raw column/row attributes, matching aiex.dma_channel_reset and aiex.dma_configure_task. The op now takes Index:$tile with a getTileOp() accessor, and both the verifier and the lowering read the column and row from the tile op. Because aie.tile carries its own verifier that bounds the coordinates against the device, core_reset no longer needs a separate out of range check, so that branch and its invalid test are dropped. The lit tests declare the tile at device scope and pass it by value.
…channel_reset Add on-board npu-xrt tests that drive the merged reset ops (Xilinx#3375, Xilinx#3370) instead of raw register writes, as op-based counterparts of the core/ and dma/ families: - core_reset_op: aiex.core_reset supplies the reset->unreset pulse on CORE_CONTROL (0x32000, bit 1) = XAie_CoreReset + XAie_CoreUnreset. Because the core has run to aie.end it is no longer enabled, so the test adds a masked re-enable of bit 0 mirroring XAie_CoreEnable -- making the whole test the driver's XAie_CoreReset -> XAie_CoreUnreset -> XAie_CoreEnable sequence, every write masked to one field. On-board, the op alone leaves the core halted -- pinning the op's documented still-enabled assumption. - dma_channel_reset_op: aiex.dma_channel_reset is a drop-in for dma/'s four reset writes on DMA_MM2S_0_Ctrl (0x1DE10, bit 1) = XAie_DmaChannelReset; the re-push BD + lock re-arm remain around it. Both ops lower to a masked maskwrite32 pulse on the same register and reset bit the raw siblings write, matching the aie-rt driver (all masked field writes). The ops previously had only aie-opt FileCheck coverage; these are their first on-silicon tests. Each op-based test compiles its raw sibling's test.cpp directly (%S/../core, %S/../dma) rather than duplicating the host harness. All five local_reset families pass on npu2 (Strix).
What this adds
A runtime-sequence op that resets a compute core, lowering to a masked pulse of the CORE_CONTROL register
reset bit (
npu.maskwrite32). It mirrorsaiex.set_lock: a non-blocking control op with no synchronizationguarantees, meant to sit inside an
aie.runtime_sequence.Why it is needed
The runtime sequence is the per-dispatch control program. Today it can configure and start DMA transfers,
and it can set a lock (
aiex.set_lock), but it has no way to reset the compute core. That gap is the reasonfor this op.
The motivating case is re-arming a resident kernel across dispatches on aie2p. A core that is started once
and left resident, so its program can run again on the next dispatch without a fresh PDI load, carries its
processor state from the previous dispatch. The heavy way to return it to a known state is a full
load_pdireload of the device image, which re-initializes everything and defeats the point of keeping the core
resident. This op is the scoped alternative: reset just the core, in place, from the control program.
What the reset does and does not touch
CORE_CONTROL.RESET resets the processor: program counter and registers. It does not clear the tile's data
memory (that is the whole point, the resident data stays put), and it does not touch locks or the tile DMA.
So this op is the core-state half of a dispatch-boundary re-arm; a caller that needs a full clean handshake
also resets the relevant locks (
aiex.set_lock) and DMA, the same way aie-rt re-initializes thoseseparately around a core reset. It also assumes the resident core is still enabled: the pulse preserves the
ENABLE bit but does not set it, so a core that has already cleared ENABLE needs a separate re-enable. I have
kept those as separate concerns rather than folding enable/lock/DMA into this one op, so it stays a single
well-defined primitive.
Why an op rather than a raw write32, and why the pulse is masked
A raw
npu.write32to CORE_CONTROL works but is opaque, and a plain write of the reset bit would clobber therest of the register. CORE_CONTROL packs ENABLE (bit 0) and RESET (bit 1) in one word. So the lowering emits
the reset as two
npu.maskwrite32writes masked to bit 1 (assert, then deassert), which preserves ENABLEinstead of overwriting it. This mirrors aie-rt's
XAie_CoreResetandXAie_CoreUnreset, which drive thereset bit with a
MaskWrite32.One deliberate difference from the sibling
dma_channel_reset: that op computes its register address throughthe target model (
getDmaControlAddress), which varies by tile class, channel, and direction. CORE_CONTROLdoes not vary, it sits at tile-local offset
0x32000on every generation (XAIE2PGBL_CORE_MODULE_CORE_CONTROLon aie2p,
XAIEMLGBL_CORE_MODULE_CORE_CONTROLon aie2, both0x32000), and there is no existinggetCoreControlAddresson the target model. Rather than add a virtual that returns the same constant forevery override, the lowering uses the constant directly with a comment tying it to the aie-rt register name.
If maintainers would prefer the symmetry with
getDmaControlAddress, I am happy to addgetCoreControlAddress(col, row)toAIETargetModelinstead.Notes for review
XAie_CoreReset, which errors on any non core tile), coordinates outside the device array (the column androw are raw attributes, so this is the only thing that bounds them), and AIE1 (the NPU runtime sequence has
no meaning there, matching
set_lock).aie2p and aie2, with
--implicit-check-notguarding against a stray unmasked write) and an invalid testthat checks the verifier rejects mem, shim, out-of-range, and AIE1.
resident objectFIFO cores resets and restarts them without disturbing the result (a two-stage streaming
design keeps producing correct output, reproduced). A control that instead clears ENABLE on one of those
cores breaks the run, which confirms the writes reach the core and that masking the pulse to the reset bit
(preserving ENABLE) is what keeps it correct. The attr-form
maskwrite32also survives the full lowering toan xclbin, so the op needs no absolute-address form. The one case I did not exercise is a run-once core that
has cleared ENABLE after reaching Done; re-arming that also needs a re-enable, as the description notes.
attribute that emits this reset automatically. That is a lifecycle design question. This PR is the missing
mechanism.