search

Xilinx / mlir-air

MIT License
71 stars 26 forks source link

`XRTBackend` Implementation of `AirBackend` is Confusing #630

Closed hunhoffe closed 2 weeks ago

hunhoffe commented 3 weeks ago

I was trying to use the compile() and load() methods of XRTBackend when I was doing some debugging recently. I realized the load() method takes a module: air.ir.Module as an argument which is then never used.

This is confusing.

The abstract base class (AirBackend) is flexible, because we can specify a unique CompiledArtifact for XRTBackend. I think this confusion would be fixed if the CompiledArgument for the XRTBackend were something like (in pseudo code):

class XRTArtifact:
  xclbin: File(),
  insts: File(),

e.g., the compiled artifacts are a pair of files (or file paths) pointing to the xclbin and instruction file.

I'm happy to make a PR for this, if others think this is a reasonable change. If there is some history behind the current format that needs to be taken into account, I'm happy to hear it!

fifield commented 3 weeks ago

I'm happy to make a PR for this, if others think this is a reasonable change. If there is some history behind the current format that needs to be taken into account, I'm happy to hear it!

There is no history other than the simplest thing was implemented. Any improvements are welcome!

hunhoffe commented 2 weeks ago

Great! Closing because I've created a PR for consideration: https://github.com/Xilinx/mlir-air/pull/634