[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

6719–6724

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/include/mlir/ExecutionEngine/SparseTensorRuntime.h

-//===- SparseTensorUtils.h - SparseTensor runtime support lib ---*- C++ -*-===//
+//===- SparseTensorRuntime.h - SparseTensor runtime support lib -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This header file provides the enums and functions which comprise the
-// public API of the `ExecutionEngine/SparseTensorUtils.cpp` runtime
+// public API of the `ExecutionEngine/SparseTensorRuntime.cpp` runtime
 // support library for the SparseTensor dialect.
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef MLIR_EXECUTIONENGINE_SPARSETENSORUTILS_H
-#define MLIR_EXECUTIONENGINE_SPARSETENSORUTILS_H
+#ifndef MLIR_EXECUTIONENGINE_SPARSETENSORRUNTIME_H
+#define MLIR_EXECUTIONENGINE_SPARSETENSORRUNTIME_H
 
 #include "mlir/ExecutionEngine/CRunnerUtils.h"
 #include "mlir/ExecutionEngine/SparseTensor/Enums.h"
 
 #include <cinttypes>
 #include <complex>
 #include <vector>
 
#define DECL_CONVERTFROMMLIRSPARSETENSOR(VNAME, V)                             \
   MLIR_CRUNNERUTILS_EXPORT void convertFromMLIRSparseTensor##VNAME(            \
       void *tensor, uint64_t *pRank, uint64_t *pNse, uint64_t **pShape,        \
       V **pValues, uint64_t **pIndices);
 MLIR_SPARSETENSOR_FOREVERY_V(DECL_CONVERTFROMMLIRSPARSETENSOR)
 #undef DECL_CONVERTFROMMLIRSPARSETENSOR
 
 } // extern "C"
 
-#endif // MLIR_EXECUTIONENGINE_SPARSETENSORUTILS_H
+#endif // MLIR_EXECUTIONENGINE_SPARSETENSORRUNTIME_H

mlir/lib/ExecutionEngine/CMakeLists.txt

 # Exclude these from libMLIR.so because the JIT infrastructure
 # is a big dependency which most don't need.
 
 set(LLVM_OPTIONAL_SOURCES
   AsyncRuntime.cpp
   CRunnerUtils.cpp
   CudaRuntimeWrappers.cpp
-  SparseTensorUtils.cpp
+  SparseTensorRuntime.cpp
   ExecutionEngine.cpp
   Float16bits.cpp
   RocmRuntimeWrappers.cpp
   RunnerUtils.cpp
   OptUtils.cpp
   JitRunner.cpp
   )
 

 set_property(TARGET mlir_float16_utils PROPERTY CXX_STANDARD 17)
 target_compile_definitions(mlir_float16_utils PRIVATE mlir_float16_utils_EXPORTS)
 
 add_subdirectory(SparseTensor)
 
 add_mlir_library(mlir_c_runner_utils
   SHARED
   CRunnerUtils.cpp
-  SparseTensorUtils.cpp
+  SparseTensorRuntime.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()

mlir/lib/ExecutionEngine/SparseTensorRuntime.cpp

-//===- SparseTensorUtils.cpp - Sparse Tensor Utils for MLIR execution -----===//
+//===- SparseTensorRuntime.cpp - SparseTensor runtime support lib ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a light-weight runtime support library for

 //      tensors. These methods can be used by any external runtime that wants
 //      to interact with MLIR compiler-generated code.
 //
 // In both cases (I) and (II), the SparseTensorStorage format is externally
 // only visible as an opaque pointer.
 //
 //===----------------------------------------------------------------------===//
 
-#include "mlir/ExecutionEngine/SparseTensorUtils.h"
+#include "mlir/ExecutionEngine/SparseTensorRuntime.h"
 
 #ifdef MLIR_CRUNNERUTILS_DEFINE_FUNCTIONS
 
 #include "mlir/ExecutionEngine/SparseTensor/COO.h"
 #include "mlir/ExecutionEngine/SparseTensor/ErrorHandling.h"
 #include "mlir/ExecutionEngine/SparseTensor/File.h"
 #include "mlir/ExecutionEngine/SparseTensor/Storage.h"
 

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",
         "include/mlir/ExecutionEngine/SparseTensor/CheckedMul.h",
         "include/mlir/ExecutionEngine/SparseTensor/Enums.h",
         "include/mlir/ExecutionEngine/SparseTensor/ErrorHandling.h",
         "include/mlir/ExecutionEngine/SparseTensor/File.h",
         "include/mlir/ExecutionEngine/SparseTensor/Storage.h",
     ],
     includes = ["include"],
     deps = [":mlir_float16_utils"],
 )
 
-# We relist Enums.h because SparseTensorUtils.h includes/reexports it
+# We relist Enums.h because SparseTensorRuntime.h includes/reexports it
 # as part of the public API.
 cc_library(
     name = "_mlir_c_runner_utils",
     srcs = [
         "lib/ExecutionEngine/CRunnerUtils.cpp",
-        "lib/ExecutionEngine/SparseTensorUtils.cpp",
+        "lib/ExecutionEngine/SparseTensorRuntime.cpp",
     ],
     hdrs = [
         "include/mlir/ExecutionEngine/CRunnerUtils.h",
         "include/mlir/ExecutionEngine/Msan.h",
         "include/mlir/ExecutionEngine/SparseTensor/Enums.h",
-        "include/mlir/ExecutionEngine/SparseTensorUtils.h",
+        "include/mlir/ExecutionEngine/SparseTensorRuntime.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",
 )