This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Ability to build CAPI dylibs from out of tree projects against installed LLVM.
ClosedPublic

Authored by stellaraccident on Oct 9 2021, 10:37 PM.

Details

Summary
  • Incorporates a reworked version of D106419 (which I have closed but has comments on it).
  • Extends the standalone example to include a minimal CAPI (for registering its dialect) and a test which, from out of tree, creates an aggregate dylib and links a little sample program against it. This will likely only work today in *static* MLIR builds (until the TypeID fiasco is finally put to bed). It should work on all platforms, though (including Windows - albeit I haven't tried this exact incarnation there).
  • This is the biggest pre-requisite to being able to build out of tree MLIR Python-based projects from an installed MLIR/LLVM.
  • I am rather nauseated by the CMake shenanigans I had to endure to get this working. The primary complexity, above and beyond the previous patch is because (with no reason given), it is impossible to export target properties that contain generator expressions... because, of course it isn't. In this case, the primary reason we use generator expressions on the individual embedded libraries is to support arbitrary ordering. Since that need doesn't apply to out of tree (which import everything via FindPackage at the outset), we fall back to a more imperative way of doing the same thing if we detect that the target was imported. Gross, but I don't expect it to need a lot of maintenance.
  • There should be a relatively straight-forward path from here to rebase libMLIR.so on top of this facility and also make it include the CAPI.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Oct 9 2021, 10:37 PM
mlir/cmake/modules/AddMLIR.cmake
63

Can you document what ENABLE_AGGREGATION does here? is there a reason why we shouldn't always do this (other than that it requires an OBJLIB?)

stephenneuendorffer requested changes to this revision.Oct 12 2021, 12:47 PM
stephenneuendorffer added inline comments.
mlir/cmake/modules/AddMLIR.cmake
282–283

Need some checks here in case the library was not exported with the right information.

This revision now requires changes to proceed.Oct 12 2021, 12:47 PM

Better detection for import and comments.

stellaraccident marked an inline comment as done.Oct 12 2021, 8:32 PM
stellaraccident added inline comments.
mlir/cmake/modules/AddMLIR.cmake
63

Done and added a TODO to just make it a default. I had originally tried to do that, but there is cruft in a number of the in-tree libraries that keep them from being built with an object library. I'd like to do a second pass and clean all of those up and then switch this to a DISABLE flag (i.e. may want to disable when just building a single-use runtime library, for example -- need to futz with that a bit).

I think this can go in tree now.

This revision is now accepted and ready to land.Oct 12 2021, 9:08 PM
This revision was landed with ongoing or failed builds.Oct 13 2021, 6:46 PM
This revision was automatically updated to reflect the committed changes.

That is weird. It appears to be something odd in the debug logging the new
rule does (which I left enabled for everyone). I think we can just comment
that out with a note that it can be enabled if needed.

I don't know much about this project. If you have it buildable, would you
mind testing my theory. Comment out the two commands here:
https://github.com/llvm/llvm-project/blob/b6c218d4fdb74c0ee467e078721438c3396dc599/mlir/cmake/modules/AddMLIR.cmake#L342

kwk added a subscriber: kwk.Oct 19 2021, 8:28 AM
kwk added inline comments.
mlir/cmake/modules/AddMLIR.cmake
366

@stellaraccident is this really needed for anything? I mean which part is supposed to consume these *.o files?

mlir/cmake/modules/AddMLIR.cmake
366

The add_mlir_aggregate uses them when constructing a shared library for a slice of the dependencies. Installing them supports out of tree projects (building from an installed LLVM) and allows them to build such things in the same way that we do in-tree. This is used today for downstream Python and Haskell bindings, both of which build a subset of the MLIR dependencies to bundle for their native package manager. In the future, it could allow out of tree projects to roll their own libMLIR.so equivalent, which is a feature that keeps getting asked for (i.e. as far as I understand it, a lot of folks have workflows where one team/CI produces the LLVM installs for all of their architectures and then multiple "downstream" teams build MLIR based things on top of them).

I assume you are asking because this universally adds bulk to the installs. I had considered gating this with a CMake option (i.e. MLIR_INSTALL_AGGREGATE_OBJECTS), which would let us scope the cost to just the folks who have this use case.

kwk added inline comments.Oct 19 2021, 10:00 AM
mlir/cmake/modules/AddMLIR.cmake
366

Thank you for the explanation.

I assume you are asking because this universally adds bulk to the installs.

That is correct. All of a sudden /usr/lib64/objects-RelWithDebInfo/obj.MLIRCAPI* files appeared that were installed but not packaged. I've asked my question because I'd like to understand where to put the files.

I had considered gating this with a CMake option (i.e. MLIR_INSTALL_AGGREGATE_OBJECTS), which would let us scope the cost to just the folks who have this use case.

Sure, then packagers can gradually opt in to this kind of extra files. I'd suggest to make the default true though and add a very good explanation what it does. Then we have one build failing and you're forced to make a decision as a packager. I'd rather have this than going silent without ever knowing about these extra set of files. Make sense?

mlir/cmake/modules/AddMLIR.cmake
366

Makes sense (and also thank you for work/attention to packaging).

Just to restate before rolling a patch:

  • Add a new MLIR_INSTALL_AGGREGATE_OBJECTS defaulting to true
  • Export this to the installed CMake config so we can check it and issue an appropriate error if mlir_add_aggregate is called on an install that doesn't have it.
  • Packagers (you) will flip this to false until/if they want to include these assets?