[WIP] Share BD size/stride verification between AIE and AIEX dialects (#2566)#3109
Draft
hunhoffe wants to merge 3 commits into
Draft
[WIP] Share BD size/stride verification between AIE and AIEX dialects (#2566)#3109hunhoffe wants to merge 3 commits into
hunhoffe wants to merge 3 commits into
Conversation
hunhoffe
requested review from
abisca,
andrej,
denolf,
erwei-xilinx,
fifield,
jackl-xilinx,
jgmelber,
pvasireddy-amd and
stephenneuendorffer
as code owners
andrej
requested changes
Jun 4, 2026
andrej
left a comment
andrej
left a comment
Collaborator
There was a problem hiding this comment.
Fairly certain about the inconsistency with repeatDim introduced here
Comment on lines
209
to
233
Collaborator
There was a problem hiding this comment.
Would it make sense to absorb these checks into verifyBDSizesStrides as well, and get rid of verifyStridesWraps entirely, just use the new verifyBDSizesStrides?
Comment on lines
+2267
to
+2271
| if (elemWidthBits > addressGranularityBits && inputStrides[innermost] != 1) | ||
| return forOp->emitOpError( | ||
| "For element widths larger than the address granularity (") | ||
| << (addressGranularityBits / 8) | ||
| << " bytes), innermost dim stride must be 1."; |
Collaborator
There was a problem hiding this comment.
So I'm not allowed to, e.g., pick every odd uint64_t element of an array? Why? I don't think a check like this existed in the old verifyStridesWraps.
|
|
||
| // Non-repeat dim strides must be positive when the corresponding size > 1 | ||
| // (the repeat dim, if present as the outermost, may have stride 0). | ||
| const int repeatDim = n - 1; |
Collaborator
There was a problem hiding this comment.
I don't think this is correct.
- For the
dma_memcpy_ndop, which goes through theAIEDmaToNpu.cpplowering, it is always the size at hard-coded index3that is used as both theRepeat_Countof the task and theIteration_WrapandIteration_Stepsize. Since fordma_memcpy_nd, there always is exactly one BD per task, and because the BD does not toggle any locks, this effectively makes this dimension at index 3 behave the same as the other dimensions except it also allows stride zero. - For
dma_tasks in the runtime_sequence, which go through theAIEDMATasksToNpu.cpplowering, the size and stride at hard-coded index3are used as theIteration_WrapandIteration_Stepsizeof that BD. Since this syntax allows chaining multiple BDs or BDs which toggle locks, this does not set theRepeat_Countof the task (which one would we use if there are multiple BDs? .. it wouldn't make sense). The repeat count is instead set on the DMA task itself using an attribute. - For all other BDs in the static part of the design (inside of
aie.mem,aie.memtile_dma, etc. ops), which go through theAIERT.cpplowering, I was unable to find any special treatment for any of the higher dimensions. The dimensions get assigned here. Repeat counts are explicitly set on the entire DMA task as well here.
Long story short, only hard-coded index 3 gets special treatment, and only sometimes. For those special cases, you have to pad out your dimensions up to four to make use of this behavior. I think the repeatDim = n - 1 here does not capture that.
Collaborator
|
Additional note regarding the repeat_count, I noticed that we do not call |
…linx#2566) Factors the size/stride/granularity portion of AIEX::verifyStridesWraps into a shared helper AIE::verifyBDSizesStrides, called from both DMABDOp::verify and the AIEX BD-emitting paths. This (1) lets DMABDOp accept inner-dim strides > 1 on sub-32b types when stride*elemWidth is a granularity multiple (previously rejected outright), and (2) makes DMABDOp reject sub-word innermost contiguous runs (previously silently accepted). The bfp/>32b inner-stride==1 constraint from Xilinx#2435 is preserved in the shared helper. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Unifies how DMA buffer-descriptor data-layout, iteration, and repeat are represented and verified across the dialects and the user-facing API, building on the shared size/stride verification from Xilinx#2566. Representation: - aie.dma_bd gains explicit iter_size/iter_stride/iter_current attributes; the dimensions list is now pure data layout (no magic stride-0 "repeat" dim). - aiex.npu.dma_memcpy_nd sizes/strides/offsets become 3 pure data dims plus explicit iter_size/iter_stride/iter_current and repeat_count attributes. - aie.objectfifo repeat_count converges to the "0 means a single pass" (extra replays) convention used by dma_start / dma_task / dma_memcpy_nd. Verification: - New shared AIE::verifyBDDataLayoutAndIteration is tile-type and bit-width aware, sourcing per-tile dimension counts and wrap/step/iteration register widths from new AIETargetModel getters (getDmaBdMaxDims, getDmaBd{Wrap,Step,Iter}Bits). Replaces and deletes AIEX::verifyStridesWraps. Lowering: - Both runtime-sequence paths (AIEDmaToNpu, AIEDMATasksToNPU) derive iteration and repeat from the explicit attributes instead of a padded index-3 slot. - Static aie.mem / aie.memtile_dma BDs now emit XAie_DmaSetBdIteration on both the CDODirect (AIERT) and legacy XAIE (AIETargetXAIEV2) codegen paths, closing a prior no-op gap. Both DMABDOp and NpuDmaMemcpyNd canonicalizers preserve iteration/repeat across linearization. Migrates all in-tree callers, the Python/IRON op surface (explicit iter_*/ repeat_count kwargs; taplib leading iteration dim split at the op-gen boundaries), and the lit suite. Adds coverage for explicit iteration, static-BD iteration emission, per-tile dimension limits, and repeat/iteration mutual exclusion. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Three tests added on main after this branch's base were not covered by the original migration: - dma_to_npu_subview_nd.mlir (Xilinx#3121): cut dma_memcpy_nd operands from the 4-dim [0,0,0,0]/[1,1,1,N] form to the 3 pure data dims. - hw_repeat_optimization_test.mlir (Xilinx#3115): refresh expected dma_start repeat_count values for the 0-means-once objectfifo convention (the lowering already emits repeat_count - 1; only the expectations, written under the prior 1-means-once form, were stale). - memtile_padding_test.mlir: reorder buffer/lock CHECK lines to match the current declaration order; DMA dimensions were already correct. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
hunhoffe
force-pushed
the
ehunhoff/issue-2566-dma-layout-checks
branch
from
53c1743 to
0ec5246
Compare
hunhoffe
changed the title
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
2 participants
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.
Closes #2566.
AIEX::verifyStridesWrapsalready had a careful granularity-based check for BD sizes and strides — this PR pulls the size/stride/granularity portion into a shared helperAIE::verifyBDSizesStridessoaie.dma_bd's verifier can use the same logic. Two follow-on behavior changes fall out:aie.dma_bdnow accepts inner-dim strides > 1 on sub-32b types whenstride × elemWidthis a multiple of the address granularity (e.g.memref<128xi16>, [<size=32, stride=2>]— 2 × 16b = 32b, perfectly word-aligned). The previous blanket check rejected these even though hardware can encode them.aie.dma_bdnow rejects sub-word innermost contiguous runs (e.g.memref<128xi8>, [<size=3, stride=4>, <size=2, stride=1>]— innermost run = 2 bytes). The AIEX path already caught these; the AIE path didn't.The
elemWidth > granularityconstraint added in #2435 (innermost stride must be 1 for bfp / wide types) is preserved in the shared helper, matching the silentstrides[0] = 0behavior ingetHardwareStridesWraps.