[mlir][sparse] Renaming mlir_sparse_tensor_utils library to SparseTensorRuntime

[wrengr]

Summary

The "mlir_xxx_utils" naming scheme is reserved/intended for shared libraries, whereas this library must be static due to issues of linking DLLs on Windows. So we rename the library to avoid any potential confusion. In addition we also rename the ExecutionEngine/SparseTensorUtils.{h,cpp} files to match the new library name.

[stella.stamenova]

LGTM. A couple of minor comments inline.

mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt

1

Is this comment still relevant after the rename? And if you want to keep it, can you add some details about the errors (such as using vectors, etc.)

18

I'm curious - why do we need this here? I thought all of llvm uses 17 now.

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel

6702–6707

Should the two comments match?

[wrengr]

mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt

1

I think it's still relevant/helpful to have, if only to explain the situation to future readers (like myself ;) so they don't try making the library shared just to rediscover why it doesn't work. Though my personal style is to err on the side of over-documenting things like this, so do let me know if it's out of line with MLIR/LLVM style for cmake files.

I've fleshed the comment out a bit more to explain how std::vector is the major stumbling block to making things shared

18

Until quite recently mlir_c_runner_utils explicitly restricted things to C++11, so since this is a dependency of mlir_c_runner_utils I wanted to make sure it adheres to the same restrictions. (The same reasoning goes for mlir_float16_utils.) mlir_c_runner_utils only recently switched that to C++17, though it's still an explicit restriction.

From a quick search, it doesn't look like CXX_STANDARD is set globally anywhere. The only uses I see are for mlir_c_runner_utils (and hence MLIRSparseTensorRuntime and mlir_float16_utils), and for mlir_cuda_runtime which restricts things to C++14.

[wrengr]

updating comments

[wrengr]

Renaming the ExecutionEngine/SparseTensorUtils.{h,cpp} files to match the new library name.

[wrengr]

rebase

[NathanielMcVicar]

This appears to have broken the C interface. I suppose the corresponding rename will be required there. https://lab.llvm.org/buildbot/#/builders/13/builds/26975/steps/6/logs/stdio

C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): warning C4190: '_mlir_ciface_getSparseTensorReaderNextF16' has C-linkage specified, but returns UDT 'f16' which is incompatible with C
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/Float16bits.h(35): note: see declaration of 'f16'

[stella.stamenova]

In D135613#3852960, @NathanielMcVicar wrote:

This appears to have broken the C interface. I suppose the corresponding rename will be required there. https://lab.llvm.org/buildbot/#/builders/13/builds/26975/steps/6/logs/stdio

C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/SparseTensorRuntime.h(245): warning C4190: '_mlir_ciface_getSparseTensorReaderNextF16' has C-linkage specified, but returns UDT 'f16' which is incompatible with C
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include\mlir/ExecutionEngine/Float16bits.h(35): note: see declaration of 'f16'

To be clear, Nate identified this change as the reason for the windows mlir buildbot failures.

[wrengr]

@NathanielMcVicar that function was added in D135480 which was created/landed after this differential. I'll make a note of this breakage there

[wrengr]

In D135613#3853446, @stella.stamenova wrote:

To be clear, Nate identified this change as the reason for the windows mlir buildbot failures.

How did D135613 cause the failure when that function wasn't introduced until D135480?

[stella.stamenova]

In D135613#3853454, @wrengr wrote:

In D135613#3853446, @stella.stamenova wrote:

To be clear, Nate identified this change as the reason for the windows mlir buildbot failures.

How did D135613 cause the failure when that function wasn't introduced until D135480?

Our messages crossed :O. You're right that D135480 is the cause and we were mistaken.

mlir/lib/ExecutionEngine/CMakeLists.txt

 add_mlir_library(mlir_c_runner_utils
   SHARED
   CRunnerUtils.cpp
   SparseTensorUtils.cpp
 
   EXCLUDE_FROM_LIBMLIR
 
   LINK_LIBS PUBLIC
-  mlir_sparse_tensor_utils
+  MLIRSparseTensorRuntime
   )
 set_property(TARGET mlir_c_runner_utils PROPERTY CXX_STANDARD 17)
 target_compile_definitions(mlir_c_runner_utils PRIVATE mlir_c_runner_utils_EXPORTS)
 
 add_mlir_library(mlir_runner_utils
   SHARED
   RunnerUtils.cpp
 

mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt

 # Unlike mlir_float16_utils, mlir_c_runner_utils, etc, we do *not* make
-# this a shared library: because doing so causes issues building on Windows.
-add_mlir_library(mlir_sparse_tensor_utils
+# this a shared library: because doing so causes issues when building
+# on Windows.  In particular, various functions take/return `std::vector`
+# but that class is not designed with dllimport/dllexport pragma,
+# therefore it cannot be safely/correctly used across DLL boundaries.
+# Consequently, we avoid using the "mlir_xxx_utils" naming scheme, since
+# that is reserved/intended for shared libraries only.
+add_mlir_library(MLIRSparseTensorRuntime
   File.cpp
   NNZ.cpp
   Storage.cpp
 
   EXCLUDE_FROM_LIBMLIR
 
   LINK_LIBS PUBLIC
   mlir_float16_utils
   )
-set_property(TARGET mlir_sparse_tensor_utils PROPERTY CXX_STANDARD 17)
+set_property(TARGET MLIRSparseTensorRuntime PROPERTY CXX_STANDARD 17)
 
 # To make sure we adhere to the style guide:
 # <https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers>
 check_cxx_compiler_flag(-Wweak-vtables
   COMPILER_SUPPORTS_WARNING_WEAK_VTABLES)
 if(COMPILER_SUPPORTS_WARNING_WEAK_VTABLES)
-  target_compile_options(mlir_sparse_tensor_utils PUBLIC
+  target_compile_options(MLIRSparseTensorRuntime PUBLIC
     "-Wweak-vtables")
 endif()

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel

deps = [
         ":LinalgTransforms",
         ":LinalgUtils",
         ":MemRefDialect",
         ":Pass",
         ":SCFDialect",
         ":SCFTransforms",
         ":SparseTensorDialect",
         ":SparseTensorPassIncGen",
+        ":SparseTensorRuntime",
         ":SparseTensorUtils",
         ":Support",
         ":TensorDialect",
         ":Transforms",
         ":VectorDialect",
-        ":mlir_sparse_tensor_utils",
         "//llvm:Support",
     ],
 )
 
 cc_library(
     name = "SparseTensorPipelines",
     srcs = glob(["lib/Dialect/SparseTensor/Pipelines/*.cpp"]),
     hdrs = ["include/mlir/Dialect/SparseTensor/Pipelines/Passes.h"],

 cc_binary(
     name = "libmlir_float16_utils.so",
     linkshared = True,
     linkstatic = False,
     deps = [":mlir_float16_utils"],
 )
 
 # Unlike mlir_float16_utils, mlir_c_runner_utils, etc, we do *not* make
-# this a shared library: because on the CMake side, doing so causes
-# issues when building on Windows.
+# this a shared library: because on the CMake side, doing so causes issues
+# when building on Windows.  In particular, various functions take/return
+# `std::vector` but that class is not designed with dllimport/dllexport
+# pragma, therefore it cannot be safely/correctly used across DLL boundaries.
+# Consequently, we avoid using the "mlir_xxx_utils" naming scheme,
+# since that is reserved/intended for shared libraries only.
 #
 # We relist Float16bits.h because Enums.h includes it; rather than
 # forcing all direct-dependants state that they also directly-depend
 # on :mlir_float16_utils (to satisfy the layering_check).
 cc_library(
-    name = "mlir_sparse_tensor_utils",
+    name = "SparseTensorRuntime",
     srcs = [
         "lib/ExecutionEngine/SparseTensor/File.cpp",
         "lib/ExecutionEngine/SparseTensor/NNZ.cpp",
         "lib/ExecutionEngine/SparseTensor/Storage.cpp",
     ],
     hdrs = [
         "include/mlir/ExecutionEngine/Float16bits.h",
         "include/mlir/ExecutionEngine/SparseTensor/COO.h",
cc_library(
     ],
     hdrs = [
         "include/mlir/ExecutionEngine/CRunnerUtils.h",
         "include/mlir/ExecutionEngine/Msan.h",
         "include/mlir/ExecutionEngine/SparseTensor/Enums.h",
         "include/mlir/ExecutionEngine/SparseTensorUtils.h",
     ],
     includes = ["include"],
-    deps = [":mlir_sparse_tensor_utils"],
+    deps = [":SparseTensorRuntime"],
 )
 
 # Indirection to avoid 'libmlir_c_runner_utils.so' filename clash.
 alias(
     name = "mlir_c_runner_utils",
     actual = "_mlir_c_runner_utils",
 )