Conversation
GRAN_ELEMS was hard-coded to 2 (4-byte shim granularity / 2-byte bf16) regardless of what was actually being transferred -- unreachable today since dtype_in/dtype_out are hard-coded bf16 here, but silently wrong for any future non-bf16 variant of this design. Derive the granule from dtype_in/dtype_out instead, and rename MAX_WRAP/MAX_STRIDE to SHIM_MAX_WRAP/SHIM_MAX_STRIDE: they are the ShimNOC-specific verifyStridesWraps bounds, not MemTile's or CoreTile's, both of which differ. Hoists split_run to module scope so it's testable in isolation. test_shim_gran_elems_matches_dtype fails (ImportError) without this and pins a concrete misaligned split at the old gran=2.
gemv and repeat each carried their own search for the same ShimNOC BD invariant: factor a contiguous run into (count, length) with both dims inside the 10-bit wrap field and the innermost length a whole number of 32-bit words, length maximal. Two implementations of one hardware constraint is the hazard -- amd#161 had to rebuild the invariant for repeat because gemv learned it somewhere unreachable. Moves SHIM_MAX_WRAP / SHIM_MAX_STRIDE / SHIM_ADDR_GRAN_BITS, shim_gran_elems() and split_run() into iron/common/shim_bd.py and points both operators at it. split_run keeps returning None rather than raising, because the callers legitimately disagree about it: gemv has a slower legal path and falls back, repeat does not and raises. Behaviour-preserving for repeat, and checked rather than asserted: repeat searched divisors upward, split_run searches the length downward, and the two agree on all 17997 (cols, granule) pairs over cols<3000 x granule in {1,2,4}, no-split cases included. That differential is kept as a test, alongside one that the returned split is actually emittable and one that the inner run is maximal -- a short innermost dim is legal but slow, so a regression would otherwise pass silently. shim_gran_elems also stops asserting on elements at least as wide as the granule (f64 returned max(1, 4//8) = 1 from repeat before; the gemv version would have tripped its assert). repeat takes arbitrary dtypes, gemv does not. Tests: 40/40 of amd#161 repeat, 5/5 gemv unit, 55 new. No device.
andrej
reviewed
Sep 18, 2026
andrej
left a comment
Collaborator
There was a problem hiding this comment.
Could this be an upstream change to MLIR-AIE instead, where we can access/expose the required fields from the TargetModel? And make IRON a user of it only.
Contributor
Author
|
@andrej really the job was done in Xilinx/mlir-aie#3728. So after that lands- I can wire it into the IRON later |
Collaborator
|
That's even better. Please ping here when upstream lands, and we can update this PR to make use of those changes. I'll hold off until merging until then and mark it draft until then. |
andrej
marked this pull request as draft
September 18, 2026 18:20
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.
gemvandrepeatboth split a contiguous run into shim BD wrap dimensions, each hand-rolling thesame invariant.
gemvalso carried aFIXMEasking for the bounds to come from the target model, anda granule constant that was bf16-only:
That is wrong for every other dtype it accepts. The shim's address-generation granularity is 32 bits,
so the granule is 4 elements for int8 and 1 for f32 -- an element-unit bound derived from 2 is 2x too
strict for int8 and 2x too loose for f32.
Added
iron/common/shim_bd.pywith onesplit_runhelper, and its tests._shim_gran_elems(dtype), deriving the granule from the memref's element type, mirroring whatverifyStridesWrapscomputes fromgetAddressGenGranularity(). It asserts the element width divides the granularity rather than silently truncating.Changed
gemvandrepeatboth use the shared helper.Removed
gemv's localsplit_run,MAX_WRAP/GRAN_ELEMS/MAX_STRIDE, and theFIXME.The bounds stay hard-coded, with a comment saying why:
getDmaBdWrapBits/getDmaBdStepBitshave noPython binding, and
getAddressGenGranularityhas a CAPI entry but is not bound intoaie.dialects.aie.AIETargetModel.Verified on
27cf75d:pytest iron/common/test_shim_bd.py iron/operators/gemv/test.py-> 60 passedfor the shim-BD and granule selections, covering bf16/int8/f32 granules and the split invariant.
black --checkclean. Pure arithmetic; no device. The gemv/repeat device paths are unchanged in shapebut not re-run here.
PR Merge Checklist
develcommit and pointing todevel.