-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[MLIR] Enabling external dialects to be shared libs for C API users #108253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@llvm/issue-subscribers-mlir Author: Maksim Levental (makslevental)
Disclaimer: everything below is currently re Linux but Mac/Windows can/should be discussed.
Some people would like to
This is actually possible today using What's not possible today is doing this same type of thing through the C API because currently
where What you want is
and then everything will work. Turns out an MVP patch is very small: diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index a3324705c525..700a8de26723 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -410,7 +410,11 @@ function(add_mlir_library name)
# which excludes it if the ultimate link target is excluding the library.
set(NEW_LINK_LIBRARIES)
get_target_property(CURRENT_LINK_LIBRARIES ${name} LINK_LIBRARIES)
- get_mlir_filtered_link_libraries(NEW_LINK_LIBRARIES ${CURRENT_LINK_LIBRARIES})
+ if(MLIR_LINK_MLIR_DYLIB)
+ set(NEW_LINK_LIBRARIES "MLIR")
+ else()
+ get_mlir_filtered_link_libraries(NEW_LINK_LIBRARIES ${CURRENT_LINK_LIBRARIES})
+ endif()
set_target_properties(${name} PROPERTIES LINK_LIBRARIES "${NEW_LINK_LIBRARIES}")
list(APPEND AGGREGATE_DEPS ${NEW_LINK_LIBRARIES})
set_target_properties(${name} PROPERTIES (tested/verified here). And now I open the floor for discussion; possible topics include:
cc @vchuravy @mofeing @jumerckx |
I'm definitely +1 on getting the dylib layering sound here (at least on Linux/MacOS -- I think that all of LLVM's dylib machinery is broken on Windows still). This is in very cold cache for me, though (as a person who has historically wrestled with it). Historically, when I've worked these things, I've had to look very closely at the |
okay I changed the action to dump You're gonna have to view the raw logs because the UI takes forever to load.
So what are we looking for as a failure mode for the patch? |
By default [`llvm_add_library`](https://github.com/llvm/llvm-project/blob/94837c8b5761d20310947be5d2e1e568f67e8c0c/llvm/cmake/modules/AddLLVM.cmake#L681) calls `llvm_setup_rpath`, which has the strange effect that even libs inside of `install/lib` have non-empty self-referential `RPATH`s; e.g., on Mac ``` $ llvm-install/lib/libMLIR.dylib | grep RPATH -A4 cmd LC_RPATH cmdsize 32 path @loader_path/../lib (offset 12) ``` which is bad/awkward if you want to move `libMLIR` (like into a wheel...). Now possibly we want to do this for all shlibs but I think we definitely want to do this for `libMLIR` because it should have no runtime lib deps (at least until [this](#108253) lands).
Disclaimer: everything below is currently re Linux but Mac/Windows can/should be discussed.
Some people would like to
MLIR_LINK_MLIR_DYLIB=ON
;DialectXYZ
as a dylib using artifacts of step 1;libDialectXYZ.so
.This is actually possible today using
DialectPlugin
; examples/standalone/standalone-plugin basically demos/exercises this (modulo theMLIR_LINK_MLIR_DYLIB=ON
but I've tested withDYLIB=ON
too). For C++ API users there are no issues; so e.g.,mlir-opt
, being a C++ API user, has no problems becausemlir-opt
andlibMLIRStandalone.so
will both link against the samelibMLIR.so
.What's not possible today is doing this same type of thing through the C API because currently
libMLIR-C
doesn't link againstlibMLIR
even in the case ofMLIR_LINK_MLIR_DYLIB=ON
because of some logic I don't quite understand. So today what occurs iswhere
libMLIR-C.so
duplicates everything inlibMLIR.so
(typeids, etc) and so everything blows up.What you want is
and then everything will work.
Turns out an MVP patch is very small:
(tested/verified here).
And now I open the floor for discussion; possible topics include:
ENABLE_AGGREGATION
is supported on Windows?);cc @vchuravy @mofeing @jumerckx
The text was updated successfully, but these errors were encountered: