This is an archive of the discontinued LLVM Phabricator instance.

Implement Pass and Dialect plugins for mlir-opt
ClosedPublic

Authored by fmorac on Mar 28 2023, 8:03 AM.

Details

Summary

Implementation of Pass and Dialect Plugins that mirrors LLVM Pass Plugin implementation from the new pass manager.

Currently the implementation only supports using the pass-pipeline option for adding passes. This restriction is imposed by the PassPipelineCLParser variable in mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:114 that loads the parse options statically before parsing the cmd line args.

mlir-opt stanalone-plugin.mlir --load-dialect-plugin=lib/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)"

Diff Detail

Event Timeline

fmorac created this revision.Mar 28 2023, 8:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
fmorac requested review of this revision.Mar 28 2023, 8:03 AM

I don't have a lot substantial to say other than that this looks very viable to me (including the DialectPlugin). Bravo on an excellent job tracking everything down and well-manicured PR.

mlir/tools/mlir-opt/CMakeLists.txt
88

This is the key thing - good job tracking it down.

fmorac updated this revision to Diff 509082.Mar 28 2023, 11:32 AM

Can you please reupload the full diff? The newest diff just looks like formatting changes.

fmorac added a comment.EditedMar 28 2023, 11:36 AM

Can you please reupload the full diff? The newest diff just looks like formatting changes.

Sure, don't know why I thought if I updated the patch it would also apply the first one.

fmorac updated this revision to Diff 509088.Mar 28 2023, 11:45 AM

Nice!

Does it work on MacOS and Windows?

mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
2

Can you make an example of the load-pass-plugin as well?

mlir/include/mlir/IR/DialectPlugin.h
34 ↗(On Diff #509088)

Seems like unfortunate copy/paste: MLIR_PLUGIN_API_VERSION

mlir/include/mlir/Pass/PassPlugin.h
32 ↗(On Diff #509088)

I'm wondering if this is really that useful to version the plugin API when for all practical consideration we are rev-locked at the git-hash level almost. Any change to MLIR can break the plugin.

mlir/lib/Pass/PassPlugin.cpp
53 ↗(On Diff #509088)

Can you make sure there is a newline here and in the other files?

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
112

Is the std::addressof doing anything different than & here?

127

Please document these

138–140

Please replace this one instead of adding a new one, otherwise we expose to the command line some options that we actually ignore.

144

Can't you just write clOptionsConfig->setDialectPluginsCallback(registry); here?

176

I understand why you made setDialectPluginsCallback a separate step (it needs the registry), but why this one?

180–182

I may miss something, but I'm not sure why the detour here

Nice!

Does it work on MacOS and Windows?

@makslevental is testing it on MacOS and I know it builds on Windows, but I don't have a Windows box to test it.

mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
2

Sure.

mlir/include/mlir/IR/DialectPlugin.h
34 ↗(On Diff #509088)

Will change it, because I missed it.

mlir/include/mlir/Pass/PassPlugin.h
32 ↗(On Diff #509088)

I decided to keep it to state that MLIR haven't modified the API plugin registration contract, so users know that the mechanism remains unchanged, and I would think this API is going to be more stable than many other components. But we can remove it.

mlir/lib/Pass/PassPlugin.cpp
53 ↗(On Diff #509088)

Sure.

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
112

It's necessary because cl::opt overrides operator&.

127

Will do.

138–140

Ok I'll change it to:

void registerCLOptions(DialectRegistry &registry);
144

Yes, I was unsure it -> initialized the object, but I just checked and it does.

176

I saw the structure of the constructor and I preferred to outline the code so the body of the constructor remained cleaner, but I can inline it.

180–182

Don't know either, there's no need for it, I'll change it.

fmorac updated this revision to Diff 509171.Mar 28 2023, 4:36 PM
fmorac marked 7 inline comments as done.
fmorac updated this revision to Diff 509309.Mar 29 2023, 4:47 AM
fmorac edited the summary of this revision. (Show Details)
rriddle added inline comments.Apr 2 2023, 10:55 PM
mlir/examples/standalone/lib/Standalone/StandalonePasses.cpp
17–18
22–37

These should generally get defined within an anonymous namespace in the file

44–45
46

It'd be cleaner to signalPassFailure here .

49–50
mlir/examples/standalone/standalone-plugin/standalone-plugin.cpp
25–31

Why define this in a separate function, instead of inline below?

40–43

Same here

mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
5

Can you use 2-space indents here?

mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
5

Same here

mlir/include/mlir/IR/DialectPlugin.h
27 ↗(On Diff #509309)
35 ↗(On Diff #509309)

Please use camelCase for variable names

58 ↗(On Diff #509309)

Why not StringRef here? Also please use camelCase for variable names.

103–104 ↗(On Diff #509309)

struct results in C always feel problematic for composability, did you consider using out-param appraoch?

mlir/include/mlir/Pass/PassPlugin.h
43 ↗(On Diff #509309)

Same comments as the other file, e.g. re: camelCase variables.

fmorac updated this revision to Diff 510541.Apr 3 2023, 9:58 AM
fmorac marked 14 inline comments as done.Apr 3 2023, 10:11 AM

The build is failing on Windows, due to an unrelated flang test see this other review: https://reviews.llvm.org/D147262 .

mlir/examples/standalone/standalone-plugin/standalone-plugin.cpp
25–31

Traditionally I've seen LLVM pass plugins split like that, but there's no good reason to not inlining them, so I did.

mlir/include/mlir/IR/DialectPlugin.h
58 ↗(On Diff #509309)

The function internally requires the string to be null terminated, I could create the string inside the function, however I consider this would be an extra step.

103–104 ↗(On Diff #509309)

Not really because the struct is simple and LLVM do it like this for their plugins. But I'm open to change it.

The build is failing on Windows, due to an unrelated flang test see this other review: https://reviews.llvm.org/D147262 .

The build seems to be working fine here. I was wondering how you fixed the windows build issue.

fmorac marked an inline comment as done.Apr 5 2023, 4:13 PM

When I submitted the last diff Phabricator applied the patch on the latest commit at the time of submission, and apparently the flang bug had been resolved by then.

rriddle accepted this revision.Apr 5 2023, 4:14 PM
This revision is now accepted and ready to land.Apr 5 2023, 4:14 PM
fmorac added a comment.Apr 6 2023, 6:44 AM

Hi could someone help me land this commit? I forgot to signoff, my info is: "Fabian Mora <fabianmcg@users.noreply.github.com>" or "Fabian Mora <fmorac@udel.edu>". Thank you!

This revision was automatically updated to reflect the committed changes.

Don't we fix forward for bazel failures?

I apologize - I saw the failure and didn't realize it was bazel specific and didn't want to leave broken. My bad, let me reland.

mehdi_amini added inline comments.Apr 6 2023, 3:08 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

It's not clear to me why this file is in "IR"? Seems like something that should be part of the OptMainLib?

mlir/lib/Pass/PassPlugin.cpp
53 ↗(On Diff #511449)

(same for this file)

fmorac added inline comments.Apr 6 2023, 3:27 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

When I was writing it I wasn't sure where to put these files, however my thinking process was that this functionality might be extended in the future beyond mlir-opt. For example mlir-translate could be modified to load dialect plugins, thus enabling plugin conversions and dialects at runtime. Also, these files in LLVM are kept in similar places, though this particular file has no equivalent in LLVM so with this one in particular I was unsure.

rriddle added inline comments.Apr 6 2023, 3:29 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

I'm not convinced we should be following LLVM here. Let's move these out of IR/. OptMainLib seems fine initially, we can hone in on a better place in followups.

mehdi_amini added inline comments.Apr 6 2023, 3:30 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

To me these are clearly not functionality of the IR or the Pass system. They are really command line tooling, you can also create a new library under the Tools/ folder if it needs to be shared.

fmorac added inline comments.Apr 6 2023, 3:34 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

Sure, what do you think about Tools/Plugins ?

mehdi_amini added inline comments.Apr 6 2023, 3:34 PM
mlir/include/mlir/IR/DialectPlugin.h
106 ↗(On Diff #511449)

That would be fine with me

fmorac updated this revision to Diff 511561.EditedApr 6 2023, 4:50 PM

Moved the Plugin related files to include/mlir/Tools/Plugins and lib/Tools/Plugins, and added the MLIRPluginsLib library.

The standalone test is now broken on my Mac:

Failed to load dialect plugin from '/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so'. Request ignored.
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error:
Could not load library '/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so': dlopen(/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so, 0x0009): tried: '/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so' (no such file), '/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so' (no such file)PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /build/bin/mlir-opt /mlir/examples/standalone/test/Standalone/standalone-plugin.mlir --load-dialect-plugin=/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so --pass-pipeline=builtin.module(standalone-switch-bar-foo)
 #0 0x0000000104d3ce98 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/build/bin/mlir-opt+0x1002a0e98)
 #1 0x0000000104d3b14c llvm::sys::RunSignalHandlers() (/build/bin/mlir-opt+0x10029f14c)
 #2 0x0000000104d3d538 SignalHandler(int) (/build/bin/mlir-opt+0x1002a1538)
 #3 0x00000001ae5982a4 (/usr/lib/system/libsystem_platform.dylib+0x1803fc2a4)
 #4 0x00000001ae569cec (/usr/lib/system/libsystem_pthread.dylib+0x1803cdcec)
 #5 0x00000001ae4a22c8 (/usr/lib/system/libsystem_c.dylib+0x1803062c8)
 #6 0x0000000106bac7f0 llvm::Expected<mlir::DialectPlugin>::fatalUncheckedExpected() const (/build/bin/mlir-opt+0x1021107f0)
 #7 0x0000000106bac6d4 std::__1::__function::__func<(anonymous namespace)::MlirOptMainConfigCLOptions::setDialectPluginsCallback(mlir::DialectRegistry&)::$_3, std::__1::allocator<(anonymous namespace)::MlirOptMainConfigCLOptions::setDialectPluginsCallback(mlir::DialectRegistry&)::$_3>, void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)>::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) (/build/bin/mlir-opt+0x1021106d4)
 #8 0x0000000106baeb00 llvm::cl::list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, llvm::cl::parser<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>::handleOccurrence(unsigned int, llvm::StringRef, llvm::StringRef) (/build/bin/mlir-opt+0x102112b00)
 #9 0x0000000104ca42cc llvm::cl::ParseCommandLineOptions(int, char const* const*, llvm::StringRef, llvm::raw_ostream*, char const*, bool) (/build/bin/mlir-opt+0x1002082cc)
#10 0x0000000106babc54 mlir::MlirOptMain(int, char**, llvm::StringRef, mlir::DialectRegistry&, bool) (build/bin/mlir-opt+0x10210fc54)
#11 0x0000000104a9e414 main (build/bin/mlir-opt+0x100002414)
#12 0x00000001ae23fe50

Thoughts on this?

I believe it's caused by the first line in mlir/examples/standalone/test/Standalone/standalone-plugin.mlir :

// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s

so it uses the wrong extension. I don't have a mac to further test it (I might be able to borrow one), so the quickest fix is to deactivate the test on darwin.

The standalone test is now broken on my Mac:

Failed to load dialect plugin from '/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.so'. Request ignored.
...

Thoughts on this?

Should be libStandalonePlugin.dylib right?

I believe it's caused by the first line in mlir/examples/standalone/test/Standalone/standalone-plugin.mlir :

// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s

so it uses the wrong extension. I don't have a mac to further test it (I might be able to borrow one), so the quickest fix is to deactivate the test on darwin.

Looks like it should be

// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s

according to https://github.com/llvm/llvm-project/blob/c98372725b5a3e56bb1c003ef9981d644b79bce7/mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_1.mlir#L6

Looks like it should be

// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s

according to https://github.com/llvm/llvm-project/blob/c98372725b5a3e56bb1c003ef9981d644b79bce7/mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_1.mlir#L6

yeah you're right, also I believe you said there was an extra flag need it. What's the way to solve this, do I just update the diff or do I need to create a new review?

We need a new review (you should only reuse a review if the commit is reverted).

There are a few different issues though:

  • the assert is a problem: we shouldn't assert because it indicates an issue with the error checking.
  • I tried the solution you mentioned, but we're still missing something in how we link the plugin I think:
******************** TEST 'STANDALONE :: Standalone/standalone-pass-plugin.mlir' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/mehdi_amini/projects/upstream/build/bin/mlir-opt /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir --load-pass-plugin=/Users/mehdi_amini/projects/upstream/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.dylib --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | /Users/mehdi_amini/projects/upstream/build/bin/FileCheck /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
--
Exit Code: 2

Command Output (stderr):
--
<unknown>:0: error: MLIR Textual PassPipeline Parser:1:1: error: 'standalone-switch-bar-foo' does not refer to a registered pass or pass pipeline
standalone-switch-bar-foo
^

FileCheck error: '<stdin>' is empty.
FileCheck command line:  /Users/mehdi_amini/projects/upstream/build/bin/FileCheck /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir

--

********************
Testing:  0.. 10.. 20.. 30.. 40
FAIL: STANDALONE :: Standalone/standalone-plugin.mlir (7 of 7)
******************** TEST 'STANDALONE :: Standalone/standalone-plugin.mlir' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/mehdi_amini/projects/upstream/build/bin/mlir-opt /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir --load-dialect-plugin=/Users/mehdi_amini/projects/upstream/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.dylib --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | /Users/mehdi_amini/projects/upstream/build/bin/FileCheck /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
--
Exit Code: 2

Command Output (stderr):
--
LLVM ERROR: can't create Attribute 'mlir::StringAttr' because storage uniquer isn't initialized: the dialect was likely not loaded, or the attribute wasn't added with addAttributes<...>() in the Dialect::initialize() method.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /Users/mehdi_amini/projects/upstream/build/bin/mlir-opt /Users/mehdi_amini/projects/upstream/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir --load-dialect-plugin=/Users/mehdi_amini/projects/upstream/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.dylib --pass-pipeline=builtin.module(standalone-switch-bar-foo)
1.	MLIR Parser: custom op parser 'builtin.module'
2.	MLIR Parser: custom op parser 'func.func'
 #0 0x00000001028b0e98 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/mehdi_amini/projects/upstream/build/bin/mlir-opt+0x1002a0e98)
 #1 0x00000001028af14c llvm::sys::RunSignalHandlers() (/Users/mehdi_amini/projects/upstream/build/bin/mlir-opt+0x10029f14c)
 #2 0x00000001028b1538 SignalHandler(int) (/Users/mehdi_amini/projects/upstream/build/bin/mlir-opt+0x1002a1538)
 #3 0x00000001ae5982a4 (/usr/lib/system/libsystem_platform.dylib+0x1803fc2a4)
 #4 0x00000001ae569cec (/usr/lib/system/libsystem_pthread.dylib+0x1803cdcec)
 #5 0x00000001ae4a22c8 (/usr/lib/system/libsystem_c.dylib+0x1803062c8)
 #6 0x000000010a3dba50 llvm::report_fatal_error(llvm::Twine const&, bool) (/Users/mehdi_amini/projects/upstream/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.dylib+0x4fa50)
 #7 0x000000010a4b4498 bool llvm::function_ref<bool (mlir::StorageUniquer::BaseStorage const*)>::callback_fn<mlir::detail::StringAttrStorage* mlir::StorageUniquer::get<mlir::detail::StringAttrStorage, llvm::StringRef&, mlir::NoneType&>(llvm::function_ref<void (mlir::detail::StringAttrStorage*)>, mlir::TypeID, llvm::StringRef&, mlir::NoneType&)::'lambda'(mlir::StorageUniquer::BaseStorage const*)>(long, mlir::StorageUniquer::BaseStorage const*) (/Users/mehdi_amini/projects/upstream/build/tools/mlir/test/Examples/standalone/lib/libStandalonePlugin.dylib+0x128498)

The patch FYI:

diff --git a/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir b/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
index 5af4b3d92e34..868e22fe03bb 100644
--- a/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
+++ b/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --load-pass-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
+// RUN: mlir-opt %s --load-pass-plugin=%standalone_libs/libStandalonePlugin%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
 
 module {
   // CHECK-LABEL: func @foo()
diff --git a/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir b/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
index 3f935db8910c..0bdbfb50334e 100644
--- a/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
+++ b/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
+// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
 
 module {
   // CHECK-LABEL: func @foo()
diff --git a/mlir/examples/standalone/test/lit.cfg.py b/mlir/examples/standalone/test/lit.cfg.py
index 3e4ceee3865c..601ac8f769f9 100644
--- a/mlir/examples/standalone/test/lit.cfg.py
+++ b/mlir/examples/standalone/test/lit.cfg.py
@@ -30,6 +30,7 @@ config.test_source_root = os.path.dirname(__file__)
 config.test_exec_root = os.path.join(config.standalone_obj_root, 'test')
 
 config.substitutions.append(('%PATH%', config.environment['PATH']))
+config.substitutions.append(('%shlibext', config.llvm_shlib_ext))
 
 llvm_config.with_system_environment(
     ['HOME', 'INCLUDE', 'LIB', 'TMP', 'TEMP'])
diff --git a/mlir/examples/standalone/test/lit.site.cfg.py.in b/mlir/examples/standalone/test/lit.site.cfg.py.in
index 9a81c6b48343..eae93d0b122e 100644
--- a/mlir/examples/standalone/test/lit.site.cfg.py.in
+++ b/mlir/examples/standalone/test/lit.site.cfg.py.in
@@ -4,6 +4,7 @@ config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@")
 config.mlir_obj_dir = "@MLIR_BINARY_DIR@"
 config.enable_bindings_python = @MLIR_ENABLE_BINDINGS_PYTHON@
 config.standalone_obj_root = "@STANDALONE_BINARY_DIR@"
+config.llvm_shlib_ext = "@SHLIBEXT@"
 
 import lit.llvm
 lit.llvm.initialize(lit_config, config)

We need a new review (you should only reuse a review if the commit is reverted).

There are a few different issues though:

  • the assert is a problem: we shouldn't assert because it indicates an issue with the error checking.
  • I tried the solution you mentioned, but we're still missing something in how we link the plugin I think:

I think @makslevental made it work by changing mlir/examples/standalone/standalone-plugin/CMakeLists.txt :

get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
get_property(conversion_libs GLOBAL PROPERTY MLIR_CONVERSION_LIBS)
set(LIBS
        MLIRStandalone
        )

add_mlir_dialect_library(StandalonePlugin
        SHARED
        standalone-plugin.cpp

        DEPENDS
        MLIRStandalone
        )

llvm_update_compile_flags(StandalonePlugin)
target_link_libraries(StandalonePlugin PRIVATE ${LIBS} "$<$<PLATFORM_ID:Darwin>:-undefined dynamic_lookup>")

mlir_check_all_link_libraries(StandalonePlugin)

I'll create the new review with both of these changes.

It does not work for me, I get the same result. I also got a warning while building: ld: warning: -undefined dynamic_lookup may not work with chained fixups

It does not work for me, I get the same result. I also got a warning while building: ld: warning: -undefined dynamic_lookup may not work with chained fixups

I know he managed to get it to work on Mac, so there must be another flag that it's missing,, I think it works with shared libs. Officially LLVM Plugins are only supported on linux, because it's the only platform in which they reliably work, so I'll try to figure out what's missing, but maybe we should put a disclaimer and disable the test for non linux platforms even when we find the right flags.

This does the trick for me

diff --git a/mlir/examples/standalone/lib/Standalone/CMakeLists.txt b/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
index 0e2d043665c3..4193bfda83e4 100644
--- a/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
+++ b/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
@@ -4,15 +4,15 @@ add_mlir_dialect_library(MLIRStandalone
         StandaloneOps.cpp
         StandalonePasses.cpp
 
         ADDITIONAL_HEADER_DIRS
         ${PROJECT_SOURCE_DIR}/include/Standalone
 
         DEPENDS
         MLIRStandaloneOpsIncGen
         MLIRStandalonePassesIncGen
 
-	LINK_LIBS PUBLIC
-	MLIRIR
-        MLIRInferTypeOpInterface
-        MLIRFuncDialect
+#		LINK_LIBS PUBLIC
+#		MLIRIR
+#        MLIRInferTypeOpInterface
+#        MLIRFuncDialect
 	)
diff --git a/mlir/examples/standalone/standalone-plugin/CMakeLists.txt b/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
index 961a3ea2c906..0c28a2e52a9f 100644
--- a/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
+++ b/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
@@ -1,22 +1,22 @@
 get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 get_property(conversion_libs GLOBAL PROPERTY MLIR_CONVERSION_LIBS)
 set(LIBS
-        MLIRIR
-        MLIRPass
-        MLIRPluginsLib
+#        MLIRIR
+#        MLIRPass
+#        MLIRPluginsLib
         MLIRStandalone
-        MLIRTransformUtils
+#        MLIRTransformUtils
         )
 
 add_mlir_dialect_library(StandalonePlugin
         SHARED
         standalone-plugin.cpp
 
         DEPENDS
         MLIRStandalone
         )
 
 llvm_update_compile_flags(StandalonePlugin)
-target_link_libraries(StandalonePlugin PRIVATE ${LIBS})
+target_link_libraries(StandalonePlugin PRIVATE ${LIBS} "$<$<PLATFORM_ID:Darwin>:-undefined dynamic_lookup>")
 
 mlir_check_all_link_libraries(StandalonePlugin)

Key thing being not linking *anything* in mlir/examples/standalone/lib/Standalone/CMakeLists.txt (was getting "pass is not registered" before I made that change).

I don't know the right way to wire this up - I guess something needs to be a LLVM_ATTRIBUTE_WEAK symbol but it would be around mlir::standalone::registerPasses and that's in generated code.

Ah yes clearly the plugins shouldn't statically link anything and resolve all of their symbols through the main binary I believe.

(I don't really follow how you'd want to involve a "weak" annotation)

(I don't really follow how you'd want to involve a "weak" annotation)

just purely going off of the wiki definition (and following along with @fmorac's use of them in standalone-plugin.cpp)

During linking, a strong symbol can override a weak symbol of the same name.

So I was imagining some situation where the symbols around e.g. StorageUniquer and mlir::Pass were marked weak and thus would work in cases where that code does need to be statically linked and alternatively would be overriden by strong symbols in mlir-opt for plugins. But maybe I've completely misunderstood the point of that attribute. And anyway it's not workable since it would take changing how stuff is generated so I guess CMake hacking is the only way?

So I was imagining some situation where the symbols around e.g. StorageUniquer and mlir::Pass were marked weak and thus would work in cases where that code does need to be statically linked and alternatively would be overriden by strong symbols in mlir-opt for plugins. But maybe I've completely misunderstood the point of that attribute. And anyway it's not workable since it would take changing how stuff is generated so I guess CMake hacking is the only way?

I think the best route is to modify those CMakes. Here's a full patch (It works on Linux):

diff --git a/mlir/examples/standalone/lib/Standalone/CMakeLists.txt b/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
index 0e2d043665c3..54fab25bcf52 100644
--- a/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
+++ b/mlir/examples/standalone/lib/Standalone/CMakeLists.txt
@@ -1,3 +1,18 @@
+if (APPLE)
+add_mlir_dialect_library(MLIRStandalone
+        StandaloneTypes.cpp
+        StandaloneDialect.cpp
+        StandaloneOps.cpp
+        StandalonePasses.cpp
+
+        ADDITIONAL_HEADER_DIRS
+        ${PROJECT_SOURCE_DIR}/include/Standalone
+
+        DEPENDS
+        MLIRStandaloneOpsIncGen
+        MLIRStandalonePassesIncGen
+	)
+else()
 add_mlir_dialect_library(MLIRStandalone
         StandaloneTypes.cpp
         StandaloneDialect.cpp
@@ -16,3 +31,4 @@ add_mlir_dialect_library(MLIRStandalone
         MLIRInferTypeOpInterface
         MLIRFuncDialect
 	)
+endif()
diff --git a/mlir/examples/standalone/standalone-plugin/CMakeLists.txt b/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
index 961a3ea2c906..bb76374a11f6 100644
--- a/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
+++ b/mlir/examples/standalone/standalone-plugin/CMakeLists.txt
@@ -1,5 +1,10 @@
 get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 get_property(conversion_libs GLOBAL PROPERTY MLIR_CONVERSION_LIBS)
+if (APPLE)
+set(LIBS
+        MLIRStandalone
+        )
+else()
 set(LIBS
         MLIRIR
         MLIRPass
@@ -7,6 +12,7 @@ set(LIBS
         MLIRStandalone
         MLIRTransformUtils
         )
+endif()
 
 add_mlir_dialect_library(StandalonePlugin
         SHARED
@@ -17,6 +23,6 @@ add_mlir_dialect_library(StandalonePlugin
         )
 
 llvm_update_compile_flags(StandalonePlugin)
-target_link_libraries(StandalonePlugin PRIVATE ${LIBS})
+target_link_libraries(StandalonePlugin PRIVATE ${LIBS} "$<$<PLATFORM_ID:Darwin>:-undefined dynamic_lookup>")
 
 mlir_check_all_link_libraries(StandalonePlugin)
diff --git a/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir b/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
index 5af4b3d92e34..868e22fe03bb 100644
--- a/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
+++ b/mlir/examples/standalone/test/Standalone/standalone-pass-plugin.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --load-pass-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
+// RUN: mlir-opt %s --load-pass-plugin=%standalone_libs/libStandalonePlugin%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
 
 module {
   // CHECK-LABEL: func @foo()
diff --git a/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir b/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
index 3f935db8910c..0bdbfb50334e 100644
--- a/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
+++ b/mlir/examples/standalone/test/Standalone/standalone-plugin.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin.so --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
+// RUN: mlir-opt %s --load-dialect-plugin=%standalone_libs/libStandalonePlugin%shlibext --pass-pipeline="builtin.module(standalone-switch-bar-foo)" | FileCheck %s
 
 module {
   // CHECK-LABEL: func @foo()
diff --git a/mlir/examples/standalone/test/lit.cfg.py b/mlir/examples/standalone/test/lit.cfg.py
index 3e4ceee3865c..601ac8f769f9 100644
--- a/mlir/examples/standalone/test/lit.cfg.py
+++ b/mlir/examples/standalone/test/lit.cfg.py
@@ -30,6 +30,7 @@ config.test_source_root = os.path.dirname(__file__)
 config.test_exec_root = os.path.join(config.standalone_obj_root, 'test')
 
 config.substitutions.append(('%PATH%', config.environment['PATH']))
+config.substitutions.append(('%shlibext', config.llvm_shlib_ext))
 
 llvm_config.with_system_environment(
     ['HOME', 'INCLUDE', 'LIB', 'TMP', 'TEMP'])
diff --git a/mlir/examples/standalone/test/lit.site.cfg.py.in b/mlir/examples/standalone/test/lit.site.cfg.py.in
index 9a81c6b48343..eae93d0b122e 100644
--- a/mlir/examples/standalone/test/lit.site.cfg.py.in
+++ b/mlir/examples/standalone/test/lit.site.cfg.py.in
@@ -4,6 +4,7 @@ config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@")
 config.mlir_obj_dir = "@MLIR_BINARY_DIR@"
 config.enable_bindings_python = @MLIR_ENABLE_BINDINGS_PYTHON@
 config.standalone_obj_root = "@STANDALONE_BINARY_DIR@"
+config.llvm_shlib_ext = "@SHLIBEXT@"
 
 import lit.llvm
 lit.llvm.initialize(lit_config, config)

Why does the linux build needs the extra dependencies?

Why does the linux build needs the extra dependencies?

The immediate answer is that the building process fails without them (probably a missing flag, that tells the linker to go ahead without trying to resolve those symbols). I'll comeback with a better answer.