Page MenuHomePhabricator

[mlir] Allow out-of-tree python building from installed MLIR.
ClosedPublic

Authored by stellaraccident on Oct 10 2021, 12:17 PM.

Details

Summary
  • Depends on D111504, which provides the boilerplate for building aggregate shared libraries from installed MLIR.
  • Adds a full-fledged Python example dialect and tests to the Standalone example (need to do a bit of tweaking in the top level CMake and lit tests to adapt better to if not building with Python enabled).
  • Rips out remnants of custom extension building in favor of pybind11_add_module which does the right thing.
  • Makes python and extension sources installable (outputs to src/python/${name} in the install tree): Both Python and C++ extension sources get installed as downstreams need all of this in order to build a derived version of the API.
  • Exports sources targets (with our properties that make everything work) by converting them to INTERFACE libraries (which have export support), as recommended for the forseeable future by CMake devs. Renames custom properties to start with lower-case letter, as also recommended/required (groan).
  • Adds a ROOT_DIR argument to declare_mlir_python_extension since now all C++ sources for an extension must be under the same directory (to line up at install time).
  • Need to validate against a downstream or two and adjust, prior to submitting.

Downstreams will need to adapt by:

  • Remove absolute paths from any SOURCES for declare_mlir_python_extension (I believe all downstreams are just using ${CMAKE_CURRENT_SOURCE_DIR} here, which can just be ommitted). May need to set ROOT_DIR if not relative to the current source directory.
  • To allow further downstreams to install/build, will need to make sure that all C++ extension headers are also listed under SOURCES for declare_mlir_python_extension.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Oct 10 2021, 12:17 PM
mlir/examples/standalone/CMakeLists.txt
26–30

So in my build (and I'm not quite sure why), this fails to find Python3, even though it's found at the toplevel.

mlir/examples/standalone/test/python/lit.local.cfg
2

I think the python tests need to be disabled if python is not found.

mlir/examples/standalone/CMakeLists.txt
26–30

Likely cmake version dependent. Once Mike lands these fixes in the main project, I'd like to put them in a macro for all to use. Consider that these bits will be replaced before this patch is ready to land.

mlir/examples/standalone/CMakeLists.txt
39

I'm a little concerned about the TARGET check here. seems like this would be better off relying on an explicit configuration variable like MLIR_ENABLE_PYTHON_BINDINGS exported from MLIRConfig.cmake?

mlir/examples/standalone/CMakeLists.txt
39

Agreed. Let me rework the entire standalone top level cmake file. I honestly forgot about it before sending the patch (and had just worked on it early to get everything building).

mikeurbach added inline comments.Oct 11 2021, 10:43 PM
mlir/examples/standalone/CMakeLists.txt
26–30

I think I'm seeing the same thing actually, which we also saw in the failures on this PR: https://github.com/llvm/torch-mlir/pull/357. I didn't see this when having MLIR and the downstream (CIRCT in my case) look for Development, but I do when looking for Development.Module. I noticed this on my laptop when I tried adopting the add_mlir_python_extension part of this patch and removed MLIR_BINDINGS_PYTHON_LOCK_VERSION.

In the container I'm testing for building manylinux wheels, I have no problem if MLIR and CIRCT both look for Development.Module.

mikeurbach added inline comments.Oct 11 2021, 10:55 PM
mlir/examples/standalone/CMakeLists.txt
26–30

I realized in the CIRCT project, if I remove REQUIRED from the find_package command, it doesn't fail, and the CMake command succeeds in general. It does say: -- Could NOT find Python3 (missing: Development.Module) (found suitable version "3.8.8", minimum required is "3.6") in the CIRCT CMake output, but I can still build the extension just fine.

mikeurbach added inline comments.Oct 12 2021, 9:44 AM
mlir/examples/standalone/CMakeLists.txt
26–30

Just to close the loop on this, I found what I was doing wrong. In my local setup, another external project other than CIRCT was searching for Development while both MLIR and CIRCT searched for Development.Module. I would have expected this to work, but in my CMake debug output, I see first MLIR finds Development.Module, then my other project finds Development, then CIRCT fails to find Development.Module. Perhaps there is a bug in CMake's FindPython caching or something. Regardless, as long as all project agree to find Development.Module, which is what I think we always want, there shouldn't be a problem. Does that help your situation @stephenneuendorffer?

Use MLIR_ENABLE_BINDINGS_PYTHON to gate python config.

stellaraccident marked 3 inline comments as done.Oct 12 2021, 7:58 PM
stellaraccident added inline comments.
mlir/examples/standalone/CMakeLists.txt
26–30

In https://reviews.llvm.org/D111689 I converted all of this in the main project to a macro and then just use that here. Should be exactly the same now.

39

I updated to export the MLIR_ENABLE_BINDINGS_PYTHON var in the MLIR installation and then use that here and elsewhere to gate. Downstreams can have their own property if they want but we should have the option of knowing whether MLIR was built with this support.

This revision is now accepted and ready to land.Oct 12 2021, 9:11 PM
mikeurbach accepted this revision.Oct 13 2021, 10:27 PM

Rebase and fix test.

Update test check.

Reverter, build was broken with:

CMake Error at /usr/share/cmake-3.20/Modules/FindPython/Support.cmake:3315 (add_library):
  Cannot find source file:
    /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/python/vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/python/lib/PythonTestModule.cpp

https://lab.llvm.org/buildbot/#/builders/61/builds/17448