This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Implements concatenate operation for sparse tensor
ClosedPublic

Authored by Peiming on Aug 4 2022, 1:57 PM.

Details

Summary

This patch implements the conversion rule for operation introduced in https://reviews.llvm.org/D131200.
Also contains integration test for correctness

Diff Detail

Event Timeline

Peiming created this revision.Aug 4 2022, 1:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Aug 4 2022, 1:57 PM
Peiming updated this revision to Diff 450866.Aug 8 2022, 10:28 AM

rebasing to main

Peiming updated this revision to Diff 450870.Aug 8 2022, 10:36 AM

fix formating issue

aartbik added inline comments.Aug 10 2022, 9:49 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
158

looks like something is missing after "from an already converted ...."

167

You use loc only once, so just inline op->getLoc() here
(and I think you have plans to restructure that later anyway)

170

same here, already converted ....

179

You can use a bit more comments here, saying you first fill the sizes array from sparse/dense source, and then adjust it for the concatenation

292

no line break in such a small method

Peiming updated this revision to Diff 452294.Aug 12 2022, 1:43 PM
Peiming marked 5 inline comments as done.

Address comments

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

I actually did not change it. Does the current version look more clear to you?

last few nits

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

Yeah, you are right. Something strange happened to the comment along the way, or perhaps it was never right.
This reads better!

One last request, since we have several "sizesFrom" (plural), it feels like this "sizeFrom" (singular) should be placed at L119, i.e. singular before plurals, just so that perhaps in the future we can share more code

593

Also add a comment what this method does, before the TODOs ;-)

641

period or : at end

666

period at end

676

period at end

1229

1st, 2nd -> (1) (2) for readability

1232

These comments use a lot of vertical space. Since you already spelled out a loop nest in the loop generator above
I would make this a bit more concise like

for i,j,k // dense_b

coo->add(adjustForOffset(i,j,k), b[i,j,k])

for elem // sparse_c

coo->add(adjustForOffset(elem.indices), elem.value)

or something like that

That way, the actual code (L1257!) is a little "closer" :-)

Peiming updated this revision to Diff 453080.Aug 16 2022, 11:31 AM
Peiming marked 7 inline comments as done.

Address comments

aartbik accepted this revision.Aug 16 2022, 12:10 PM

ship it!

This revision is now accepted and ready to land.Aug 16 2022, 12:10 PM
This revision was landed with ongoing or failed builds.Aug 16 2022, 1:47 PM
This revision was automatically updated to reflect the committed changes.