This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] refine lexicographic insertion to any tensor
ClosedPublic

Authored by aartbik on Nov 16 2021, 1:14 PM.

Details

Summary

First version was vectors only. With some clever "path" insertion,
we now support any d-dimensional tensor. Up next: reductions too

Diff Detail

Event Timeline

aartbik created this revision.Nov 16 2021, 1:14 PM
aartbik requested review of this revision.Nov 16 2021, 1:14 PM
bixia accepted this revision.Nov 17 2021, 2:46 PM
This revision is now accepted and ready to land.Nov 17 2021, 2:46 PM
wrengr added inline comments.Nov 17 2021, 2:52 PM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
295–299

style

373

I like removing the need for the sparsity parameter. But for legibility/clarity, add a method bool isCompressedDim(uint64_t) to encapsulate this idiom everywhere it's used. (And/or bool isDenseDim(uint64_t) depending on how you expect Singleton and other DimLevelTypes to go)

422

Excellent! I've been wanting to factor this function out of fromCOO for a while, just hadn't gotten around to it yet :)

436–437

Since the goal of that definition for d is to reverse the iteration order and i is used nowhere else, it would be cleaner to just iterate d directly.

463

dimension.

wrengr requested changes to this revision.Nov 17 2021, 2:52 PM
This revision now requires changes to proceed.Nov 17 2021, 2:52 PM
aartbik marked 4 inline comments as done.Nov 17 2021, 3:57 PM
aartbik added inline comments.
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
373

Agreed, more readable in the long run and it will trigger the singleton work changes needed.

422

We probably can refactor more shared code, but this was rather tricky to get right, so let's tread carefully. ;-)

436–437

I don't disagree, but ending at zero with an unsigned made the iteration a bit awkward....

aartbik updated this revision to Diff 388055.Nov 17 2021, 4:00 PM
aartbik marked 3 inline comments as done.

addressed comments

aartbik updated this revision to Diff 388058.Nov 17 2021, 4:04 PM
aartbik marked an inline comment as done.

one last comment

wrengr accepted this revision.Nov 17 2021, 5:11 PM
wrengr added inline comments.
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
436–437

Doh, I missed that. I still think phrasing things as counting down makes more sense though. Could always rephrase things as a while-loop...

uint64_t rank = getRank();
assert diff < rank;
uint64_t d = rank - 1;
while (true) {
    ...stuff...
    if (d == diff) break;
    --d;
}

Though that does start looking pretty gross too. Actually, while writing that up I just noticed you'll want to have that assertion regardless of whether rephrasing the loop, since otherwise rank - diff will do bad things.

This revision is now accepted and ready to land.Nov 17 2021, 5:11 PM
aartbik updated this revision to Diff 388076.Nov 17 2021, 5:35 PM
aartbik marked an inline comment as done.

added asserts on rank

aartbik added inline comments.Nov 17 2021, 5:42 PM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
436–437

Added the assert.