This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Renaming mlir_sparse_tensor_utils library to SparseTensorRuntime
ClosedPublic

Authored by wrengr on Oct 10 2022, 12:41 PM.

Details

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.

Diff Detail

Event Timeline

wrengr created this revision.Oct 10 2022, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 12:41 PM
wrengr requested review of this revision.Oct 10 2022, 12:41 PM

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?

This revision is now accepted and ready to land.Oct 10 2022, 12:44 PM
wrengr marked an inline comment as done.Oct 10 2022, 1:24 PM
wrengr added inline comments.
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 updated this revision to Diff 466601.Oct 10 2022, 1:25 PM

updating comments

wrengr updated this revision to Diff 466604.Oct 10 2022, 1:42 PM

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

wrengr edited the summary of this revision. (Show Details)Oct 10 2022, 1:42 PM
aartbik accepted this revision.Oct 11 2022, 9:36 AM

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'

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.

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

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?

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.