This is an archive of the discontinued LLVM Phabricator instance.

"[mlir][sparse]Rename SparseUtils.cpp library to SparseTensorUtils.cpp"
AbandonedPublic

Authored by HarrietAkot on Nov 2 2021, 2:50 AM.

Details

Reviewers
aartbik
Summary

The sparse compiler currently uses a “one size fits all” sparse runtime support library (in lieu of elaborate codegen for those parts, which could be an alternative approach).

The code lives at:

https://github.com/llvm/llvm-project/blob/main/mlir/lib/ExecutionEngine/SparseUtils.cpp

However, it has been suggested that "SparseTensorUtils" would be a better name for this library.

Diff Detail

Event Timeline

HarrietAkot created this revision.Nov 2 2021, 2:50 AM
HarrietAkot requested review of this revision.Nov 2 2021, 2:50 AM
xgupta added a subscriber: xgupta.Nov 2 2021, 4:21 AM

[mlir][sparse] tag should be in beginning, see https://llvm.org/docs/DeveloperPolicy.html#commit-messages. There should be a summary of "why" you have created this patch or renamed the file. You can do so from the Edit revision button available at the top right corner.

[mlir][sparse] tag should be in beginning, see https://llvm.org/docs/DeveloperPolicy.html#commit-messages. There should be a summary of "why" you have created this patch or renamed the file. You can do so from the Edit revision button available at the top right corner.

Thank you @xgupta for your feedback. I shall work on this

HarrietAkot retitled this revision from "Rename SparseUtils.cpp library to SparseTensorUtils.cpp[mlir][sparse]" to "[mlir][sparse]Rename SparseUtils.cpp library to SparseTensorUtils.cpp".Nov 2 2021, 5:38 AM
HarrietAkot edited the summary of this revision. (Show Details)
HarrietAkot edited the summary of this revision. (Show Details)
aartbik requested changes to this revision.EditedNov 2 2021, 8:24 AM

You cannot just rename the cpp file, you will also have to tell the build system about this change (as was alluded to in the bugzilla entry).

To make sure your patch works, please use " -DMLIR_INCLUDE_INTEGRATION_TESTS=ON" in your configuration setup.
Then at the minimum run the "cmake --build . --target check-mlir-integration" command. In this case you would have seen:

CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):

Found unknown source file SparseTensorUtils.cpp

Please update
/<path>/llvm-project/mlir/lib/ExecutionEngine/CMakeLists.txt
This revision now requires changes to proceed.Nov 2 2021, 8:24 AM

Please also add a reference to https://bugs.llvm.org/show_bug.cgi?id=52304 in the summary.

Thank you. I will make these changes

aartbik added inline comments.Nov 2 2021, 8:49 AM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
1

Also note that you will have to change this name, please make sure the /-=== -===/ keeps on lining up with our 80-col requirement

Please abandon this revision now that you moved this into a new revision (also see my comments in that new one)

HarrietAkot abandoned this revision.Nov 2 2021, 2:03 PM