Skip to content

mem_copy: the passThrough kernel is compiled for the wrong element type - #160

Open
atassis wants to merge 1 commit into
amd:develfrom
atassis:pr/mem-copy-bit-width
Open

mem_copy: the passThrough kernel is compiled for the wrong element type#160
atassis wants to merge 1 commit into
amd:develfrom
atassis:pr/mem-copy-bit-width

Conversation

@atassis

@atassis atassis commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

mem_copy's non-bypass path types its line buffers bf16 (design.py:179) and links
aie_kernels/generic/passThrough.cc, which picks its element type from a BIT_WIDTH
macro the caller must supply. get_kernel_artifacts passes no extra_flags, so
BIT_WIDTH is undefined, the preprocessor evaluates it as 0, and the #else // 32
branch compiles. mha/op.py:105 is the same kernel done correctly, with
extra_flags=["-DBIT_WIDTH=16"]; those two are its only consumers.

The template copies one v64uint8 -- 64 bytes -- per N elements, so N is what ties
the trip count to the element width: 32 for int16, 16 for int32. Compiled as
passThrough_aie<int32_t, 16> but handed a bf16 element count, it runs width/16
iterations of 64 bytes = width * 4 bytes against a buffer holding width * 2.

Read off the built artifact rather than inferred:

$ llvm-objdump -t build/mem_copy.o | grep passThrough_aie
_Z15passThrough_aieIiLi16EEvPT_S1_ii          # passThrough_aie<int, 16>

and in the generated MLIR the objectFifo buffers are memref<64xbf16> -- 128 bytes --
while the call passes lineWidth = 64, so each one has 4 x 64 = 256 bytes written
through it. 128 bytes past the end, into whatever the allocator placed next.

The second half, which the first was hiding

Setting the flag makes one arm hang, deterministically, 3 of 3:
input_length=1024, num_cores=16 gives a 64-element tile, which at the correct N=32 is
two loop iterations -- against the AIE_LOOP_MIN_ITERATION_COUNT(6) the loop
declares. The int32 miscompile was running four, still under six but evidently enough to
survive, so the wrong element type was masking a false promise underneath it.

The kernel cannot make that promise: the trip count is (height * width) / N with all
three supplied by the caller. Removed rather than lowered.

Added

Changed

  • iron/operators/mem_copy/op.py: pass -DBIT_WIDTH=16, matching the bf16 line type.
  • aie_kernels/generic/passThrough.cc: drop the unverifiable minimum-trip-count promise.

Removed

Evidence

67/67 pass -- 64 mem_copy arms including the extensive ones, plus mha, the kernel's other
consumer. The object now carries passThrough_aie<short, 32> and each call moves 128
bytes.

64/64 mem_copy arms also passed before this change, which is the point rather than a
reassurance: the overrun is past the region the tests compare, so nothing in the suite
could see it.

Dropping the pipelining hint costs nothing measurable at the largest shape
(8192 elements, one core, 256 iterations): median 181.6 us against 177.7 us over 12 reps
each, inside a 140-245 / 123-491 spread. Per-dispatch overhead dominates that
measurement, so it bounds the cost rather than showing it is zero.

design.py types the line buffers bf16, but get_kernel_artifacts passes no extra_flags,
so passThrough.cc's BIT_WIDTH is undefined, evaluates to 0, and the int32 branch
compiles. The template moves one 64-byte vector per N elements -- 32 for int16, 16 for
int32 -- so given a bf16 element count it runs width/16 iterations and writes width*4
bytes through a buffer holding width*2. Confirmed on the artifact: the object carried
passThrough_aie<int, 16>, and the generated call passes lineWidth = 64 against
memref<64xbf16>, i.e. 256 bytes through 128. mha/op.py is the same kernel done right.

Setting the flag then exposes what it was hiding: at the correct N=32 mem_copy's
smallest tile is two iterations, against the AIE_LOOP_MIN_ITERATION_COUNT(6) the loop
declared, and the design hangs. The kernel cannot promise a trip count its caller
supplies, so the assertion is removed rather than lowered.

67/67 on device, mem_copy plus mha. They also passed before, which is the point: the
overrun is past the compared region.

@andrej andrej left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change in principle, but need to know about performance implications.

Also, would it be much extra work to allow other data types and pass the right bit width depending on input dtype in op.py?

And, one remark: We might end up deciding to hold PRs that modify kernels here until Xilinx/mlir-aie#3599 is merged in MLIR-AIE, then ask to migrate the changes there rather than here. Just as a forward-looking warning.

Comment on lines -23 to 25
AIE_LOOP_MIN_ITERATION_COUNT(6)
// No minimum trip count: the caller supplies all of height, width and N. The 6 that
// was asserted here is false for mem_copy's 64-element bf16 tile, which runs two
// iterations at N=32 and hangs on device.
for (int j = 0; j < (height * width); j += N) // Nx samples per loop
{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This likely has a performance implication. Have you compared the generated assembly with and without? Could you please paste below?

If they don't change with/without this macro, fine to delete. If not, I'd like a specialization for >6 trip count and one without, so we can get max performance for large buffers. We use this for performance measurements of bandwidth, so it is important.

Also note

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants