Page MenuHomePhabricator

[MLIR] Fixes for BUILD_SHARED_LIBS=on
AbandonedPublic

Authored by stephenneuendorffer on Jan 13 2020, 12:02 AM.

Details

Summary

This patch enables the above cmake option, which builds most libraries
as DLLs instead of statically linked libraries. The main effect of this is
that incrememntal build times are greatly reduced, since usually only one
library need be relinked in response to isolated code changes.

The bulk of this patch is fixing incorrect usage of cmake, where library
dependencies are listed under add_dependencies rather than under
target_link_libraries or under the LINK_LIBS tag. Correct usage should be
like this:

add_dependencies(MLIRfoo MLIRfooIncGen)
target_link_libraries(MLIRfoo MLIRlib1 MLIRlib2)

Diff Detail

Event Timeline

marbre added a subscriber: marbre.Jan 13 2020, 1:54 AM
ftynse added a subscriber: ftynse.Jan 14 2020, 11:59 PM

I think this goes into the right direction. The issue with this patch is that it removes some libraries from add_dependency for some targets, but not all of them, thus creating more inconsistency than we already have. Could you make it consistent for all targets and, if possible and in a follow up, add a doc that says how to properly set up a library for a new dialect.

nicolasvasilache accepted this revision.Jan 16 2020, 5:45 AM

Thanks! A lot of these mistakes have been carried forward by virtue of copypasta..
As Alex mentioned, would be great to have some doc and increase consistency but I see this as nice progress already.

This revision is now accepted and ready to land.Jan 16 2020, 5:45 AM

Rebased. Also cleaned up the dependencies around mlir-opt. This is tested with BUILD_SHARED_LIBS=on and BUILD_SHARED_LIBS=off

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

The changes overall looks fine to me, but this patch does not seem enough to build with ENABLE_SHARED_LIB right? We still have a cyclic dependency to break first I believe.

Also, how does this patch compare to D72554 ?

The changes overall looks fine to me, but this patch does not seem enough to build with ENABLE_SHARED_LIB right? We still have a cyclic dependency to break first I believe.

This depends on https://reviews.llvm.org/D69440. I added that as a 'parent' commit, which appears to be how phabricator represents such things.
This combined with "STATIC" annotations is enough to get BUILD_SHARED_LIB to work.

Also, how does this patch compare to D72554 ?

D72554 seems focused on a different approach to breaking the cyclic dependency problem, which does not modify the llvm cmake environment, but has the same end result (always building the libraries with cyclic dependencies as static libraries.)
Either way could work. My way (in D69440) is simpler, but we'll need an "add_mlir_library" cmake infrastructure anyway to get to having libMLIR.so.

Don't change add_dependencies lines: we'll fix those later

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

This landed in modified form as D73653