Change dma bd syntax

[fifield]

These are the dma_bd op syntax changes from #1137. This or something similar is needed to bump mlir, due to upstream printer changes where default valued optional attributes no longer get printed in the same way. e.g. the offset operand is not printed when zero. This PR moves everything other than dims and pad_dims to the attribute dictionary:

// old
aie.dma_bd(%buf01_0 : memref<16xi32>, 64, 128, [<size = 2, stride = 1>, ... ])

// new
aie.dma_bd(%buf01_0 : memref<16xi32>, dims = [<size = 2, stride = 1>, ... ]) {offset = 64 : i32, len = 128 : i32 }

[github-actions]

Coverage Report

Created: 2024-08-05 16:14

Click here for information about interpreting this report.

Filename	Function Coverage	Line Coverage	Region Coverage	Branch Coverage
home/runner/work/mlir-aie/mlir-aie/lib/Dialect/AIE/IR/AIEDialect.cpp	91.13%	82.57%	85.18%	75.63%
Totals	91.13%	82.57%	85.18%	75.63%

Generated by llvm-cov -- llvm version 14.0.0

[stephenneuendorffer]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

I'm leery of these ops turning into a string of attributes, although it's arguable that they already are.

[fifield]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

I'm leery of these ops turning into a string of attributes, although it's arguable that they already are.

Well the simplest easiest thing was to recycle the changes in the old (and now this) PR and fixup the tests over coffee. But I did want to reopen the discussion on what to do.

[fifield]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

Another option is to put a keyword on everything in the assemblyFormat string:

(`,` `offset` `=` $offset^)?  (`,` `len` `=` $len^)? ...

[stephenneuendorffer]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

Another option is to put a keyword on everything in the assemblyFormat string:

(`,` `offset` `=` $offset^)?  (`,` `len` `=` $len^)? ...

That at least elides the types compared to using the attribute list.

[andrej]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

Another option is to put a keyword on everything in the assemblyFormat string:

(`,` `offset` `=` $offset^)?  (`,` `len` `=` $len^)? ...

This option would have my vote over the attr dict. Both options seem fine though.

[jgmelber]

If I understand the LLVM change, the simplest thing is to implement a custom printer which prints the offset if the length is specified. Then we don't require a syntax change.

Another option is to put a keyword on everything in the assemblyFormat string:

(`,` `offset` `=` $offset^)?  (`,` `len` `=` $len^)? ...

This option would have my vote over the attr dict. Both options seem fine though.

+1

[fifield]

It seems like folks would be most happy with, let's call it option 3, which is keyword = value, but this still requires a small fix of the printer. In the process of learning that I cut-n-pasted the parser/printer, modified them to keep the existing syntax, and called it a day (#1660), call that option 2. So I'm closing this one (option 1), which is the least favored.