This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] refactoring SparseTensorUtils: (3 of 4) code-cleanup
ClosedPublic

Authored by wrengr on Sep 13 2022, 9:15 PM.

Details

Summary

Previously, the SparseTensorUtils.cpp library contained a C++ core implementation, but hid it in an anonymous namespace and only exposed a C-API for accessing it. Now we are factoring out that C++ core into a standalone C++ library so that it can be used directly by downstream clients (per request of one such client). This refactoring has been decomposed into a stack of differentials in order to simplify the code review process, however the full stack of changes should be considered together.

  • D133462: Part 1: split one file into several
  • D133830: Part 2: Reorder chunks within files
  • (this): Part 3: General code cleanup
  • D133833: Part 4: Update documentation

This part performs some general code cleanup including:

  • making more things const, especially for the targets of pointers
  • using preincrement wherever possible (per LLVM style guide)
  • adding messages to most assert statments.
  • moving argument casting from the core function/method definitions to the CPP wrappers

Depends On D133830

Diff Detail

Event Timeline

wrengr created this revision.Sep 13 2022, 9:15 PM
wrengr requested review of this revision.Sep 13 2022, 9:15 PM
aartbik accepted this revision.Sep 14 2022, 3:10 PM
aartbik added inline comments.
mlir/include/mlir/ExecutionEngine/SparseTensor/COO.h
78

This idiomatic change really become more personal taste than anything else (and yes, I am aware that for general iterators there is a big advantage, but not for simple integers). But since you seem to feel strongly about it... ;-)

mlir/include/mlir/ExecutionEngine/SparseTensor/Storage.h
709–716

we don't use this kind of style for if-return imlicit else construct

This revision is now accepted and ready to land.Sep 14 2022, 3:10 PM
wrengr updated this revision to Diff 460577.Sep 15 2022, 5:49 PM

rebase

mlir/include/mlir/ExecutionEngine/SparseTensor/Storage.h
709–716

I don't follow what you mean

wrengr updated this revision to Diff 463987.Sep 29 2022, 12:06 PM

Rebasing to parent D133462 at Diff 463979.

wrengr updated this revision to Diff 464002.Sep 29 2022, 12:56 PM

git clang-format