circt icon indicating copy to clipboard operation
circt copied to clipboard

[Python] Use the "private namespace" approach to include the upstream `mlir` packages within the `circt` package.

Open mikeurbach opened this issue 3 years ago • 3 comments

This was discussed on Discord nearly a year ago: https://discord.com/channels/636084430946959380/742572728787402763/873039553517215854. We just never got around to implementing it.

The Discord points out a patch where IREE adopts a similar approach: https://github.com/google/iree/pull/6668/files.

For us, I think this boils down to a couple CMake changes.

  1. adjusting the INSTALL_COMPONENT, INSTALL_DESTINATION, and OUTPUT_DIRECTORY of our CAPI library: https://github.com/llvm/circt/blob/2715c2dde8aee1dd8ea8f83c332e0a9b8b1824a8/lib/Bindings/Python/CMakeLists.txt#L132
  2. remove the CIRCTMLIRPythonModules target completely: https://github.com/llvm/circt/blob/2715c2dde8aee1dd8ea8f83c332e0a9b8b1824a8/lib/Bindings/Python/CMakeLists.txt#L149
  3. add the MLIRPythonSources.Core target under DECLARED_SOURCES for CIRCT Python modules: https://github.com/llvm/circt/blob/2715c2dde8aee1dd8ea8f83c332e0a9b8b1824a8/lib/Bindings/Python/CMakeLists.txt#L162

In essence, this lets us get replace import mlir.ir with import circt.ir. We only depend on the builtin dialect from upstream, but that woudl also become circt.dialects.builtin.

Taking control of these packages within the circt package will give us more flexibility and control. One specific use-case I have in mind, will come after https://reviews.llvm.org/D128593 is incorporated. This gives the ability for the ir.Context to be configured in a site-specific way. We could customize it so that our dialects and passes automatically come registered, which is something we have sought for a while.

I think technically we could implement that customization without doing the change in this issue, but having import mlir.ir.Context import a CIRCT-specific context seems to violate the principle of least suprise. So I'm suggesting we do this change, and update users to import circt.ir.Context. This could be done before or after the above patch is incorporated, and seems like a generally good thing to do that we just never got to. It will be somewhat painful to find and replace all users of CIRCT's Python bindings to import the relevant packages from under circt instead of mlir, but that should be a mechanical, one-time change.

@teqdruid what do you think about this? It's not strictly necessary, but I believe this is the expected way for downstreams to do things.

mikeurbach avatar Jul 06 '22 00:07 mikeurbach

+1 - most downstreams are doing this (IREE, torch-mlir, Jax that I know of). Torch-mlir is probably the simplest at this point, if looking for a template.

stellaraccident avatar Jul 06 '22 02:07 stellaraccident

I am 100% on board. Especially if we could extend it further and hide CIRCT in the pycde namespace (and have a different context) so the same python process could use CIRCT for something else.

teqdruid avatar Jul 06 '22 03:07 teqdruid

Thanks @stellaraccident and @teqdruid, I will take this and update the circt_core package. Then we can follow up with a similar move for PyCDE, although I haven't thought through what that should be yet.

mikeurbach avatar Jul 06 '22 15:07 mikeurbach