Page MenuHomePhabricator

[MLIR] Add support for

Authored by stephenneuendorffer on Jan 21 2020, 12:21 PM.



Putting this up mainly for discussion on
how this should be done. I am interested in MLIR from
the Julia side and we currently have a strong preference
to dynamically linking against the LLVM shared library,
and would like to have a MLIR shared library.

This patch adds a new cmake function add_mlir_library()
which accumulates a list of targets to be compiled into Note that not all libraries make sense to
be compiled into In particular, we want
to avoid libraries which primarily exist to support
certain tools (such as mlir-opt and mlir-cpu-runner).

Note that the resulting depends on LLVM, but
does not contain any LLVM components. As a result, it
is necessary to link with to avoid linkage
errors. So, requires LLVM_BUILD_LLVM_DYLIB=on

FYI, Currently it appears that LLVM_LINK_LLVM_DYLIB is broken
because mlir-tblgen is linked against and
and independent LLVM components.

Previous version of this patch broke depencies on TableGen
targets. This appears to be because it compiled all
libraries to OBJECT libraries (probably because cmake
is generating different target names). Avoiding object
libraries results in correct dependencies.

(updated by Stephen Neuendorffer)

Diff Detail

Event Timeline

vchuravy created this revision.Jan 21 2020, 12:21 PM
vchuravy marked an inline comment as done.
vchuravy added inline comments.
20 ↗(On Diff #239404)

This is to work around

Unit tests: fail. 62080 tests passed, 1 failed and 784 were skipped.

failed: Clang.Driver/cc-print-options.c

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Thanks for looking into this!


I think the right thing to do is to add an add_mlir_library command, similar to D72554 which is used in the other CMakeFiles, so this list does not need to be maintained separately from where the libraries are declared.

vchuravy updated this revision to Diff 243422.Feb 8 2020, 7:27 PM
vchuravy marked an inline comment as not done.

Properly implement shared library similar to clang-shlib

vchuravy retitled this revision from [mlir] Strawman PR for shared library support to [mlir] Shared library support.Feb 8 2020, 7:28 PM
vchuravy marked an inline comment as done.

comment is obsolete?


Worth a comment? The variable name here seems a little weird. Maybe "MLIR_LIBRARY_LIBS"?

1 ↗(On Diff #243422)

Seems like an unintentional addition?

1 ↗(On Diff #243422)

See note below.

1 ↗(On Diff #243422)

Should not be in

1 ↗(On Diff #243422)

Should not be in

1 ↗(On Diff #243422)

Should not be in

14 ↗(On Diff #243422)

Should not be in

1 ↗(On Diff #243422)

Should not be in


There's a few libraries which probably shouldn't be linked into, and I think this is one of them.


I think it's probably better to call add_llvm_library than add_mlir_library and then remove it later.

Do we have a path forward if we were to use MLIR in LLVM?


I suspect that these libraries need to be added with whole_archive_link, to ensure that static initializers are called appropriately. (see the mlir-opt build rules and AddMLIR.cmake)


This breaks the dependencies between .cpp files and the tablegen'd .inc files they depend on. I believe the problem is that the CMakeFiles.txt are setup to use:
add_dependencies(someLib someRuleIncGen)

However, if OBJECT is specified, then llvm_add_library creates new targets which cannot replicate those dependencies.

The best solution is probably to mass-convert all the CMakeFiles.txt to use a DEPEND line when calling add_mlir_library. This would get passed through to llvm_add_library and enable the newly created
targets (named obj.${name}) to be created with the correct dependencies.

stephenneuendorffer retitled this revision from [mlir] Shared library support to [MLIR] Add support for
stephenneuendorffer edited the summary of this revision. (Show Details)
stephenneuendorffer commandeered this revision.Feb 20 2020, 11:32 AM
stephenneuendorffer edited reviewers, added: vchuravy; removed: stephenneuendorffer.
stephenneuendorffer marked 3 inline comments as done.
vchuravy added inline comments.Feb 21 2020, 3:22 PM

I copied this from the clang shlib, so if this is indeed obsolete
we should remove it there as well.


This is copied as well from the clang shlib and so it would be great if we can fix that generally,
but I don't have easy access to a MSVC environment. I build my windows binaries with mingw.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 28 2020, 11:51 AM
This revision was automatically updated to reflect the committed changes.
stephenneuendorffer edited the summary of this revision. (Show Details)
xgupta added a subscriber: xgupta.Fri, Mar 6, 7:54 PM