search

Xilinx / mlir-air

MIT License
74 stars 26 forks source link

[WIP] Add Matrix Scalar Add Programming Example #594

Closed hunhoffe closed 2 months ago

hunhoffe commented 3 months ago

This PR adds a programming example of matrix scalar addition using channels and tiling. It includes 5 different ways to do tiling in this context.

This PR also includes some changes to the python API with the goal of providing a clean interface:

fifield commented 2 months ago
  • Wrap AllocOp and DeallocOp to clean up AllocOp interface, and then wrap DeallocOp to maintain a symmetric interface
  • Add default values for the ChannelGet/ChannelPut operations, as well as automatic conversion from python ints where it seems useful.

It would be nice to make these their own PR(s).


from air.dialects.air import Alloc

I'm a bit hesitant to wrap (i.e. diverge from) upstream operators to save a couple characters of typing. Alloc is not an air dialect op. Do we already do this to AllocOp in mlir-aie? Or maybe everything is objfifo there...

hunhoffe commented 2 months ago
  • Wrap AllocOp and DeallocOp to clean up AllocOp interface, and then wrap DeallocOp to maintain a symmetric interface
  • Add default values for the ChannelGet/ChannelPut operations, as well as automatic conversion from python ints where it seems useful.

It would be nice to make these their own PR(s).

from air.dialects.air import Alloc

I'm a bit hesitant to wrap (i.e. diverge from) upstream operators to save a couple characters of typing. Alloc is not an air dialect op. Do we already do this to AllocOp in mlir-aie? Or maybe everything is objfifo there...

I believe the aie uses AllocOp internally but users at the python level do not use it (at least, I can't find an example of direct use in the examples in the mlir-aie repo).

My thinking was that it's best not to expose options/interfaces that are basically wholly undocumented in air because if you leave them open, the only way to know how to use AllocOp/DeallocOp in this context is to copy/paste from an existing example and hope for the best - which doesn't exactly build programmer confidence.

However, I'm happy to remove/table the AllocOp/DeallocOp changes. And I'm also happy to put the ChannelGet/ChannelPut changes in a separate PR as well for further discussion.

fifield commented 2 months ago

My thinking was that it's best not to expose options/interfaces that are basically wholly undocumented in air because if you leave them open, the only way to know how to use AllocOp/DeallocOp in this context is to copy/paste from an existing example and hope for the best - which doesn't exactly build programmer confidence.

Upstream dialects and bindings are documented upstream, e.g. https://mlir.llvm.org/docs/Dialects/MemRef/, https://mlir.llvm.org/docs/Bindings/Python. Maybe usability and documentation improvements can be proposed there?

hunhoffe commented 2 months ago

Added separate PR for toggling the aircc emit-while-loop option: https://github.com/Xilinx/mlir-air/pull/613

hunhoffe commented 2 months ago

My thinking was that it's best not to expose options/interfaces that are basically wholly undocumented in air because if you leave them open, the only way to know how to use AllocOp/DeallocOp in this context is to copy/paste from an existing example and hope for the best - which doesn't exactly build programmer confidence.

Upstream dialects and bindings are documented upstream, e.g. https://mlir.llvm.org/docs/Dialects/MemRef/, https://mlir.llvm.org/docs/Bindings/Python. Maybe usability and documentation improvements can be proposed there?

Okay, yes, that makes sense! I believe I was thinking about this more as a Python programmer and less like an MLIR programmer. Since we import this operations through air.dialects... it makes me think it's an air specific python packages and not a representation of a library that exists outside of MLIR-AIR. But I believe this import structure is an artifact of the MLIR build structure (that the dialect python code is generated uniquely for this repo), so it's probably fine as-is.

hunhoffe commented 2 months ago

Closing this PR in favor of: