[PATCH] D73130: [mlir] Shared library support

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer added inline comments.


================
Comment at: mlir/cmake/modules/AddMLIR.cmake:40-41
+  else()
+    # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
+    # so we need to handle it here.
+    if(BUILD_SHARED_LIBS)
----------------
comment is obsolete?


================
Comment at: mlir/cmake/modules/AddMLIR.cmake:51
+    endif()
+    set_property(GLOBAL APPEND PROPERTY MLIR_STATIC_LIBS ${name})
+  endif()
----------------
Worth a comment? The variable name here seems a little weird.  Maybe "MLIR_LIBRARY_LIBS"?


================
Comment at: mlir/lib/Dialect/VectorOps/CMakeLists.txt.rej:1
+diff a/mlir/lib/Dialect/VectorOps/CMakeLists.txt b/mlir/lib/Dialect/VectorOps/CMakeLists.txt	(rejected hunks)
+@@ -1,13 +1,28 @@
----------------
Seems like an unintentional addition?


================
Comment at: mlir/lib/TableGen/CMakeLists.txt:1
-add_llvm_library(LLVMMLIRTableGen
+add_mlir_library(LLVMMLIRTableGen
   Argument.cpp
----------------
See note below.


================
Comment at: mlir/test/Dialect/SPIRV/CMakeLists.txt:1
-add_llvm_library(MLIRSPIRVTestPasses
+add_mlir_library(MLIRSPIRVTestPasses
   TestAvailability.cpp
----------------
Should not be in libMLIR.so


================
Comment at: mlir/test/lib/IR/CMakeLists.txt:1
-add_llvm_library(MLIRTestIR
+add_mlir_library(MLIRTestIR
   TestFunc.cpp
----------------
Should not be in libMLIR.so


================
Comment at: mlir/test/lib/Pass/CMakeLists.txt:1
-add_llvm_library(MLIRTestPass
+add_mlir_library(MLIRTestPass
   TestPassManager.cpp
----------------
Should not be in libMLIR.so


================
Comment at: mlir/test/lib/TestDialect/CMakeLists.txt:14
 
-add_llvm_library(MLIRTestDialect
+add_mlir_library(MLIRTestDialect
   TestDialect.cpp
----------------
Should not be in libMLIR.so


================
Comment at: mlir/test/lib/Transforms/CMakeLists.txt:1
-add_llvm_library(MLIRTestTransforms
+add_mlir_library(MLIRTestTransforms
   TestAllReduceLowering.cpp
----------------
Should not be in libMLIR.so


================
Comment at: mlir/tools/mlir-opt/CMakeLists.txt:14
 )
-add_llvm_library(MLIRMlirOptLib
+add_mlir_library(MLIRMlirOptLib
   mlir-opt.cpp
----------------
There's a few libraries which probably shouldn't be linked into libMLIR.so, and I think this is one of them.


================
Comment at: mlir/tools/mlir-shlib/CMakeLists.txt:16
+#  - it pollutes the global options space.
+list(REMOVE_ITEM mlir_libs "MLIRTableGen")
+
----------------
I think it's probably better to call add_llvm_library than add_mlir_library and then remove it later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73130/new/

https://reviews.llvm.org/D73130