Page MenuHomePhabricator

Implement the conversion from sparse constant to sparse tensors.
ClosedPublic

Authored by bixia on Sep 23 2021, 4:18 PM.

Details

Summary

The sparse constant provides a constant tensor in coordinate format. We first split the sparse constant into a constant tensor for indices and a constant tensor for values. We then generate a loop to fill a sparse tensor in coordinate format using the tensors for the indices and the values. Finally, we convert the sparse tensor in coordinate format to the destination sparse tensor format.

Add tests.

Diff Detail

Event Timeline

bixia created this revision.Sep 23 2021, 4:18 PM
bixia requested review of this revision.Sep 23 2021, 4:18 PM
bixia updated this revision to Diff 374860.Sep 24 2021, 8:38 AM

Rebased and resolved conflict.

Oh, this is solid work Bixia! Thank you, I love how we no longer have to iterate over a dense iteration space to initialize from a sparse constant!

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
264

You probably saw the clang-tidy message already, but our style prefers camel case for locals, e.g. indicesAttr or something like that

266

same

279

unsigned

281

unsigned

381–382

coo -> COO

425

since you use this here and below in the loop gen at L437 can we give this a meaningful name

isCOOConstant = iValues.hasValue();
if (isCOOConstant)

...
aartbik added inline comments.Sep 24 2021, 10:48 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
207

this method leaves the insertion point in the if val != 0
for subsequent code addition

Probably something to document in the comment at L198
(ie. both the generation of the if as well as the post-condition on the insertion point)

398

maybe use the informal

val = values[i]
[i1,..ik] = indices[i]
t->add(val, [i1,..ik], [p1,..,pk])

notation to make the fact that L391 assign a vector more clear

bixia marked 6 inline comments as done.Sep 24 2021, 11:43 AM
bixia updated this revision to Diff 374916.Sep 24 2021, 11:44 AM

Addressed review comments.

aartbik added inline comments.Sep 24 2021, 1:48 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
425

I don't see this change yet?

bixia marked 3 inline comments as done.Sep 24 2021, 2:19 PM
bixia updated this revision to Diff 374961.Sep 24 2021, 2:19 PM

Used bool isCOOConstant.

bixia updated this revision to Diff 374963.Sep 24 2021, 2:22 PM

Added [mlir][sparse] to PR subject.

aartbik accepted this revision.Sep 24 2021, 2:30 PM

Ship it! Great work.

This revision is now accepted and ready to land.Sep 24 2021, 2:30 PM
bixia updated this revision to Diff 374975.Sep 24 2021, 3:49 PM

resolved conflict.

bixia updated this revision to Diff 375290.Sep 27 2021, 8:42 AM

Resolved conflicts.

bixia updated this revision to Diff 375292.Sep 27 2021, 8:47 AM

Fixed PR description.