This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Making way for SparseTensorRuntime to support non-permutations
ClosedPublic

Authored by wrengr on Nov 8 2022, 5:13 PM.

Details

Summary

Systematically updates the SparseTensorRuntime to properly distinguish tensor-dimensions from storage-levels (and their associated ranks, shapes, sizes, indices, etc). With a few exceptions which are noted in the code, this ensures the runtime has all the semantic changes necessary to support non-permutations.

(Whereas operationally, since we're still using std::vector<uing64_t> to represent the mappings, there's no way to pass in any interesting non-permutations. Changing the representation to std::function will be done in a separate differential.)

Depends On D137680

Diff Detail

Event Timeline

wrengr created this revision.Nov 8 2022, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 5:13 PM
wrengr requested review of this revision.Nov 8 2022, 5:13 PM
Peiming added inline comments.Nov 10 2022, 1:57 PM
mlir/include/mlir/ExecutionEngine/SparseTensor/PermutationRef.h
86

Any reason for not naming the function as pushForward?

aartbik accepted this revision.Nov 11 2022, 3:21 PM
aartbik added inline comments.
mlir/include/mlir/ExecutionEngine/SparseTensor/Attributes.h
15

This is a nice forward looking comment!

mlir/include/mlir/ExecutionEngine/SparseTensor/File.h
225

I like with you did with the dim2lvl idea (i.e. beyond permutations). Very clear!

251

perhaps TODO to make it jump out here too?

272

spell out STS?

mlir/include/mlir/ExecutionEngine/SparseTensor/PermutationRef.h
89

I think this is all still subject to RVO for efficiency, right?

mlir/include/mlir/ExecutionEngine/SparseTensor/Storage.h
84

Thanks for adding this part of the doc, that clarifies your terminology very well for the readers of this code!

283

I wanted to ask why you removed this comment, but I see you moved it down to public place. Makes sense indeed.

mlir/lib/ExecutionEngine/SparseTensorRuntime.cpp
128

note that this (and below) may also be used by some out of tree projects

This revision is now accepted and ready to land.Nov 11 2022, 3:21 PM
wrengr updated this revision to Diff 474897.Nov 11 2022, 5:11 PM
wrengr marked 4 inline comments as done.

Addressing nits

mlir/include/mlir/ExecutionEngine/SparseTensor/File.h
251

My vim setup highlights "NOTE" the same way as "TODO", "FIXME", etc so it already jumps out for me ;)

mlir/include/mlir/ExecutionEngine/SparseTensor/PermutationRef.h
86

This transformation is standardly spelled as a single word (e.g., https://en.wikipedia.org/wiki/Pushforward_measure). Also, I feel like capitalizing it might cause folks to think it's somehow related to push_back

89

Afaict, this isn't one of the cases where C++17 *requires* copy elision, but in practice it should always be subject to NRVO.

mlir/lib/ExecutionEngine/SparseTensorRuntime.cpp
128

Is this something we specifically want to provide for out-of-tree projects? (I assume yes...) My main concern re adding the comment is just that the current API doesn't generalize to non-permutations. I can change the API, of course, but that just pushes the problem onto client code. In particular that means clients will have to provide functions for the mappings —but I'd rather avoid anything like trying to pass Python functions into the runtime.

I wasn't thinking about out-of-tree projects when I wrote the comment, so I'll update them to make my concerns a bit clearer.