Page MenuHomePhabricator

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

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



This patch implements the conversion rule for operation introduced in
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

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


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


same here, already converted ....


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


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


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

last few nits


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


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


period or : at end


period at end


period at end


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


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.