[dyn-seq P1.4] Static-vs-dynamic TXN equivalence harness#3289
Draft
hunhoffe wants to merge 25 commits into
Draft
[dyn-seq P1.4] Static-vs-dynamic TXN equivalence harness#3289hunhoffe wants to merge 25 commits into
hunhoffe wants to merge 25 commits into
Conversation
Contributor
Coverage ReportCreated: 2026-07-06 20:26Click here for information about interpreting this report.
Generated by llvm-cov -- llvm version 18.1.3 |
hunhoffe
force-pushed
the
dyn-seq-p3-aiex-to-emitc
branch
from
ef1d123 to
ae25456
Compare
…dexList) Converts aie.dma_bd from legacy positional attribute form to the declarative DynamicIndexList representation, matching aiex.npu.dma_memcpy_nd: - sizes/strides: Variadic<I64> + DenseI64ArrayAttr static_* + custom<DynamicIndexList> assemblyFormat, enabling runtime values to flow as SSA operands while keeping constants in the static array (no arith overhead for fully-static BDs). - offset/len: Optional<I32> SSA operands (same scalar-operand idiom as #3225's npu write32/push_queue ops). Constant offset/len materialize arith.constant; runtime values enable dynamic-length transfers without recompilation. - Adds arith as a dependent dialect of the AIE dialect (offset/len builders materialize arith.constant). The 157-line hand-written DMABDOp::parse/print is deleted in favor of the declarative assemblyFormat, fixing the #1919 parser ambiguity (positional optional args that silently parsed wrong combinations). New accessors: getMixedSizes/Strides, getFoldedDimensions, getConstantDimensions, getConstantOffset/Len. buildWithConstants funnels all int-taking C++ OpBuilder calls so producers (AIEObjectFifoStatefulTransform, AIELowerMemcpy, etc.) compile unchanged. On the static lowering path, getConstantDimensions folds the mixed list to a BDDimLayoutAttr array (consumer sites: AIEDMATasksToNPU, AIERT, AIETargetXAIEV2) with a clean diagnostic (not a miscompile) for runtime values. The LinearizeContiguousBDTransfer canonicalizer is updated to clear the new operand/static arrays correctly. Byte-identical gate: test/Targets/AIEGenerateXAIE (XAIEV2 register-write goldens) 37/38 pass unchanged; one pre-existing failure unrelated to this change. Closes #1919. Partial #1922 (dma_bd only; objectfifo deferred). Part of #3222. Co-Authored-By: Claude <noreply@anthropic.com>
Mechanical migration of ~312 test .mlir files from the legacy positional
aie.dma_bd syntax to the new declarative DynamicIndexList form:
Old: aie.dma_bd(%buf : memref<256xi32>, 0, 256, [<size = 16, stride = 1>])
New: aie.dma_bd(%buf : memref<256xi32> offset = %c0_i32 len = %c256_i32
sizes = [16] strides = [1])
Constants for offset/len are materialized as arith.constant in the enclosing
region (outside aiex.dma_configure_task bodies, which only allow DMABDOp,
UseLockOp, NextBDOp, and EndOp). CHECK lines using the old aie.dma_bd syntax
are updated to use the new format with {{.*}} wildcards for SSA operand names.
Migration done by utils/migrate_dma_bd_full.py (region-aware balanced-brace
scanner). Manual fixups: bad_dma_op.mlir (constant in wrong block position),
scratchpad_params (dma_configure_task_for without parens), canonicalize-mem
and canonicalize_linear_dma_bd CHECK line updates.
Co-Authored-By: Claude <noreply@anthropic.com>
Adds a dma_bd() wrapper in python/dialects/aie.py mirroring npu_dma_memcpy_nd: users pass one interleaved list per sizes/strides (ints and SSA Values mixed freely); the static/dynamic split is internal via _dispatch_mixed_values. offset/len accept ints (materialized as arith.constant i32) or SSA Values. Updates shim_dma_bd (aiex.py) and TileDma.resolve (iron/dataflow/tile_dma.py) to route through the new wrapper using sizes/strides lists instead of the removed dimensions=list(zip(...)) idiom. Co-Authored-By: Claude <noreply@anthropic.com>
Fix 1 (ship-blocker): aiex.py imported dma_bd from the raw generated module (_aie_ops_gen) instead of the aie.py wrapper. After wheel rebuild, shim_dma_bd would crash with TypeError passing plain ints for offset/len. Move dma_bd into the existing from .aie import block. Fix 2: LinearizeContiguousBDTransfer emitted a spurious error diagnostic for valid IR with runtime BD sizes (the new feature), because getFoldedDimensions was called with op.emitError() before confirming all sizes are constants. Pre-check all sizes/strides for constant-foldability; only call getFoldedDimensions (which needs them constant) after that guard passes. Fix 3: LinearizeContiguousBDTransfer injected a len operand when no len was present on a BD with an unranked memref buffer. Add an explicit early-exit for that case (can't safely infer product == full transfer). Fix 4: Padding-exceeds-len check was silently skipped for runtime len operands. Now emits a clear "not yet supported" error when padding is present and len is a non-constant operand, rather than silently omitting the safety check. Fix 5: Nested _as_i32 helper inside dma_bd() (aie.py) was a copy of the module-level helper in aiex.py. Promote to module level in aie.py with a None guard; both copies are now consistently documented as mirrors. Fix 6: Update the dependentDialects comment in AIE.td to accurately explain why dialect-level placement is correct (builder contract requires pre-loading). Fix 7: Replace the opaque emitOpError from getConstantDimensions() in the three static lowering paths (AIEDMATasksToNPU, AIERT, AIETargetXAIEV2) with an explicit up-front runtime-size check emitting a user-facing "not yet supported" message. Add mlir/Dialect/Utils/StaticValueUtils.h include. Fix 8: Change buildWithConstants offset/len parameters from int to int32_t, matching getI32IntegerAttr's parameter type and eliminating the signed-integer type-safety hole for large offset values. New tests: canonicalize_linear_dma_bd gains a section verifying runtime-sized BDs pass --canonicalize silently (no spurious error). nd-dma-pad-runtime-len.mlir pins the Fix 4 "not yet supported" diagnostic. Co-Authored-By: Claude <noreply@anthropic.com>
Black reformats three migration utility scripts in utils/. clang-format reformats two .td files touched by the SSA operand conversion. No functional changes. Co-Authored-By: Claude <noreply@anthropic.com>
AIEOps.td and AIE.td were run through clang-format, which mangled
`attr-dict` into `attr - dict` in 26 op assembly formats. mlir-tblgen
rejects that ("expected literal, variable, directive, or optional
group"), so every build job failed. TableGen files are not clang-format
targets; restore both files to keep only the intentional aie.dma_bd
DynamicIndexList change.
Co-Authored-By: Claude <noreply@anthropic.com>
…ayout Turn static_sizes/static_strides into OptionalAttr and make the printed sizes/strides clause optional, so a plain linear transfer prints as `aie.dma_bd(%buf : memref<...> offset = .. len = ..)` with no trailing `sizes = [] strides = []`. Add a buildMixed helper that dispatches mixed static/dynamic OpFoldResults through dispatchIndexOpFoldResults (leaving the static arrays unset when the layout is empty), a FoldConstantBDDimList canonicalization that folds constant size/stride operands back into the static arrays, and a verifier guard (verifyListOfOperandsOrIntegers + rank match) to avoid the getMixedValues crash on inconsistent operand/sentinel counts. Consolidate the numpy-int-aware _as_i32 helper in aie.py and route the IRON Bd/TileDma path through sizes/strides lists. Co-Authored-By: Claude <noreply@anthropic.com>
…nsfers Update the dma_bd test corpus to the printed form produced by the now-optional static sizes/strides: linear transfers drop the `sizes = [] strides = []` clause entirely. Regenerated CHECK lines from real aie-opt/aie-translate output. The AIEGenerateXAIE XAIEV2 register-write goldens are unchanged byte-for-byte, confirming constants still lower identically. Co-Authored-By: Claude <noreply@anthropic.com>
The earlier mechanical migration (929cf28) over-matched: its balanced-paren rewrite removed arith.constant %c<N> definitions from tests that live near dma_bd patterns but are otherwise unrelated (AIEVec srs/fold, bfp, packet_flow, etc.), leaving those SSA names undefined. Restore the affected files to the correct form so the shift/offset constants they reference are defined again. Co-Authored-By: Claude <noreply@anthropic.com>
With the Bd sizes/strides fields now list-typed, pyright inferred bd_kwargs as dict[str, list] from its sizes=/strides= initializer, then rejected the later int (offset/len) and tuple (packet) assignments. Give bd_kwargs an explicit dict[str, Any] annotation. Co-Authored-By: Claude <noreply@anthropic.com>
… memcpy_nd) Fixes the CI failures on PR #3306. The op previously carried offset/len as plain Optional<I32> operands, forcing an arith.constant for every static dma_bd. That broke two ways: the injected constants stranded across region boundaries in the test migration ("use of undeclared SSA value name"), and constants living inside aie.mem bodies made AIECoreToStandard's op removal erase a mem whose nested constant still had uses (eraseOp assertion). Now offset/len follow the same split sizes/strides and aiex.npu.dma_memcpy_nd already use: a compile-time constant lands in the new static_offset/static_len attribute, and only a genuine runtime value becomes the operand. A small custom<DynamicScalar> directive prints the constant form as a bare scalar (offset = 0 len = 256). getConstantOffset/Len read the attribute first; FoldConstantBDDimList folds a constant offset/len operand back into the attribute; FoldLinearBD sets static_len directly. Test .mlir files are migrated to the attribute form; four npu-xrt tests with pre-existing cross-region stranded constants are regenerated. The AIEGenerateXAIE byte-identical golden set is unchanged (constants lower identically), and the previously-failing npu-xrt tests pass on hardware. Co-Authored-By: Claude <noreply@anthropic.com>
The matrix_multiplication_using_cascade tests deduplicated their per-core
arith.constant into a single aie.mem region during the earlier operand-form
migration, stranding the memref.store uses in the sibling aie.core regions
("operand does not dominate this use"). Regenerate the four files from the
positional source so each region keeps its own constants and the dma_bds use
the attribute form.
Also correct the now-inaccurate dependent-dialect comment (offset/len no
longer materialize arith.constant) and note that the packet zero-length guard
in AIERT only covers compile-time-constant lengths.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- dma_bd() Python wrapper takes transfer_len (was len) to avoid shadowing the builtin and match shim_dma_bd; still lowers to the op's len operand. - buildWithConstants uses getI32IntegerAttr for offset/len to match AIEI32Attr, keeping the cast sign-exact for negative offsets. - Distinguish _split_i32_scalar from _as_i32 in its docstring (attribute vs materialized arith.constant). Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # test/Passes/objectfifo-liveness/sufficient.mlir # test/Passes/objectfifo-liveness/under_buffered.mlir
…n form The self-review commit renamed the dma_bd() wrapper's len kwarg to transfer_len (avoids shadowing the builtin, matches shim_dma_bd) and made a constant len land in the static_len attribute rather than an arith.constant operand. This test still passed len= and CHECK'd the operand form, so it hit "got multiple values for keyword argument 'len'". Route through transfer_len and expect the bare `len = 2`. Co-Authored-By: Claude <noreply@anthropic.com>
Apply Andrej's review suggestions (AIE.td arith dependency, dma_bd offset/len docs, buildWithConstants) and give the sibling comments the same concise treatment: drop the internal cross-references and upstream-op name-drops, keep the load-bearing "why". Co-Authored-By: Claude <noreply@anthropic.com>
Prove milestone #3222's correctness spine: a runtime_sequence compiled to a C++ TXN builder (aie-translate --aie-npu-to-cpp) produces a byte-identical TXN word stream to the production binary emitter (--aie-npu-to-binary) when invoked with matching constants, and runtime scalar args flow in as function parameters. Each test lowers a static and a dynamic sequence to terminal npu ops and does a three-way byte-identical compare: binary golden vs generate_txn_<static>() vs generate_txn_<dynamic>(N). The split localizes drift to EmitC codegen vs runtime-arg substitution. Coverage: - Both DMA lowering paths, which converge on the same terminal ops but nothing else in-tree exercises end to end: dma_memcpy_nd (memcpy_nd*.mlir) and the DMA-task path (dma_task*.mlir; dma_configure_task_for/start/await, SSA bd_id from #3225, the only path reaching the sync op). - Both device generations (npu2 + npu1 variants), since device info is baked into the header words. - Every op the C++ TXN target supports: write32, maskwrite32, blockwrite, address_patch, sync. - Two sizes on the memcpy path (N=4096 and N=8192) to exercise the add-a-size recipe and guard the runtime-arg path at more than one value. The DMA-task sequences carry aie.dma_bd in the declarative sizes/strides/offset/ len form (#3306); the C++ builder name is generate_txn_<device>_<seq>, so the comparator is parameterized against generate_txn_main_<seq>. compare_main.cpp is fully -D parameterized so one comparator serves every path, device, and size; README documents the add-a-size/add-a-pattern recipe. Co-Authored-By: Claude <noreply@anthropic.com>
hunhoffe
force-pushed
the
dyn-seq-p4-equivalence-harness
branch
from
9c99981 to
03296b5
Compare
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.
Phase 1 finale of #3222. Adds a lit harness proving the milestone's "static ≡ dynamic" criterion: a runtime_sequence produces an identical TXN stream whether its scalar fields are baked-in constants or runtime arguments.
Each test lowers a static and a dynamic sequence to npu ops, then asserts three identical word streams: the binary-emitter golden,
generate_txn_<static>(), andgenerate_txn_<dynamic>(N). The split localizes drift to EmitC codegen vs. runtime-arg substitution.Inputs/compare_main.cppreports the first divergent word and is-Dparameterized to serve every path/device/size.Coverage
dma_memcpy_nd(2 sizes)sync)Both DMA paths (the DMA-task path — SSA
bd_idfrom #3225 — is the only one exercised end-to-end to a TXN stream and the only one reachingsync), both device generations, and every supported op (write32,maskwrite32,blockwrite,address_patch,sync).Stacked on #3256.