dialects: (aie) Add aie dialect

[gabrielrodcanal]

Port of the AMD Xilinx AIE dialect for programming the AIEs on the AMD Xilinx Versal FPGA architecture. AIE is a hardened systolic array present in the Versal devices. The dialect describes netlists of AIE components and it can be lowered to the processor's assembly using the vendor's compiler. A description of the original dialect can be found here https://xilinx.github.io/mlir-aie/AIEDialect.html

Other than obvious improvements to the code the reviewers might spot, I would be interested in receiving feedback on the use of SymbolTable for DeviceOp and the reference to createObjectFifo from ObjectFifoAcquireOp. My main concern is that the user has to make sure the createObjectFifo operation is assigned to a DeviceOp before creating an ObjectFifoAcquireOp operation that refers to it. Do you see this as an acceptable restriction? If so, should we enforce this through the verifier? Find an example of what I am talking about in https://github.com/xdslproject/mlir-aie/blob/xdsl_tutorials/tutorials/tutorial-4/xdsl-tutorial-4.ipynb. In particular:

    object_fifo = aie.createObjectFifo(elem_number, tile14, tile34, i32, [256], "of")
    ...

    @Builder.region
    def region0(builder: Builder):
        ...
        builder.insert(object_fifo)
        ...
        
    device_val = IntegerAttr.from_int_and_width(0, i32)
    device = aie.DeviceOp(device_val, region0)

    acquire_fifo = aie.ObjectFifoAcquireOp(I32Attr(aie.PRODUCE_PORT), I32Attr(1), object_fifo.sym_name, device)

This forces the user to add new operations to the DeviceOp region after its creation instead of being able to insert all the operations with the builder. It looks a bit nasty to me. Example:

region0.block.add_op(core14)

I would like to request a review from @mesham, @superlopuh, @math-fehr, @AntonLydike and @PapyChacal.

BTW this doesn't pass Pyright yet, but I'd like to get some feedback before I work on fixing this.

[codecov]

Codecov Report

Attention: 393 lines in your changes are missing coverage. Please review.

Comparison is base (0edc396) 90.17% compared to head (ab87c87) 89.35%.

Files	Patch %	Lines
xdsl/dialects/experimental/aie.py	52.36%	393 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1763      +/-   ##
==========================================
- Coverage   90.17%   89.35%   -0.83%     
==========================================
  Files         305      306       +1     
  Lines       37041    37869     +828     
  Branches     5515     5591      +76     
==========================================
+ Hits        33402    33837     +435     
- Misses       2860     3253     +393     
  Partials      779      779              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superlopuh]

The symbol things look good to me. Is it the same as the aie dialect? Would it make sense to add some interop tests to check that this really is a reimplementation of their dialect exactly?

[superlopuh]

similarly here for attribute names and python vs C++ conventions, I'd recommend making python pretty and adding the translation hook thing. I'm not sure if everyone else will agree with this though, definitely a matter of personal preference

[gabrielrodcanal]

What would be your suggestion for the name of this attribute?

[compor]

Big PR; I left some comments, which I think is a good start, but more iterations to come.

[superlopuh]

Great! I think it might be worth having a go at replacing all the print without a corresponding parse with the assembly_format, but don't want to slow you down from now :)

[gabrielrodcanal]

Great! I think it might be worth having a go at replacing all the print without a corresponding parse with the assembly_format, but don't want to slow you down from now :)

Yes! I'm working on it :)

[compor]

I understand that this goes to experimental but I think it might worth revisting how we do things in general regarding this.
What would constitute "graduating" from experimental?

Considering how long this took to "finalize", I am not sure writing tests would slow down development that much.

On the flip side, we are called to review a massive PR and regain context after weeks of giving an initial review.

Considering this, I don't feel confident approving this, but I don't want to block it.
So, please go ahead without my explicit approval.

Experimental PR

[mesham]

I think this looks good - it's adding the AMD AIE dialect into xDSL via experimental and as Gabriel has actioned the specific suggestions and comments on this I think it's good to get it checked in and usable more widely. There are some actions to write tests around this in the future, but many things in experimental lack tests (and indeed the point of experimental is to get less mature things in and available sooner) so it won't be unique in this regard. As experimental is explicitly "use at your own risk, this is less mature", then I think this is fine and nice to expand the available dialects.

[tobiasgrosser]

I am fine adding this for now. Then we can go with smaller PRs that incrementally improve what we have.

[tobiasgrosser]

I would suggest submitting this through a separate PR, as this is indeed not in the experimental folder or taking this out of this PR.

[gabrielrodcanal]

I think this is a change from main, if I click on 'View file' it takes me to the merge @superlopuh did from main, so I guess there's no need for a new PR?

[tobiasgrosser]

Can you merge latest main and make sure this diff disappears?

[tobiasgrosser]

OK, that is good to go for me. I personally would add tests but that's up to you, I guess.

[gabrielrodcanal]

OK, that is good to go for me. I personally would add tests but that's up to you, I guess.

I will add tests in the near future, once I'm done with some deadlines!