This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] extend storage specifier operations for slices.

Authored by Peiming on Jan 12 2023, 3:22 PM.

Diff Detail

Event Timeline

Peiming created this revision.Jan 12 2023, 3:22 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Jan 12 2023, 3:22 PM
Peiming added inline comments.

This is enabled in the next patch

bixia added inline comments.Jan 13 2023, 3:47 PM

I am still trying to put everything into a consistent picture, but here are my questions so far:

(1) the "slice" in sparse_tensor.encoding here has three fields, offset/size/stride, but the specifier only has offset/stride (the size is represented in dim size). Maybe sparse_tensor.encoding shouldn't encode "size" info? I meant, you can use the same sparse_tensor.encoding to annotate different sizes of slices, similar to the fact that you can use the same sparse_tensor.encoding to annotate sparse tensors of the same layout but different sizes?

(2) when the "slice" in sparse_tensor.encoding is not provided, would that be equivalent to having offset/stride == 0/1 ? In this PR, we issue an error if "slice" is not in the encoding we we query offset/stride.

Peiming added inline comments.Jan 14 2023, 10:07 PM

For 1) You are right, and I thought about removing it too (it somehow overlapped with the tensor DimOp) .

But I decided to keep it for future extension, it could be used to stored the actual sizes of the underlying tensor. Then it can be used to remove the check if the slice size equals to the tensor size.

For 2), it is because I want to reduce the number of fields for non slices after lowering (so that we don’t need actual fields to store offset/stride array for non slices). We could of course skip the field for static slice but it would also require us to fill the hole when casting static slice to a dynamic one. See my comment in next patch in ExtractSliceOpConverter.

Both problems are really some design choices, and we can have further discussion on them

aartbik added inline comments.Feb 28 2023, 11:37 AM

Can you please update the doc in SparseTensorStorageLayout.h as part of this revision.
Good to keep that up to date!


We probably need to show CSR/CSR_SLICE as part of the example now
(typically, we omit CSR def since it is so widespread, but here it seems essential)


hehe, okay ;-)


period at end

Please be sure to rebase this patch onto D144773, because otherwise this is coig to cause a lot of patch conflicts.


Is this *really* for dimensions? Since everything else in StorageSpecifierKind is for levels, I'm pretty sure this should be for levels too. (N.B., the name of DimSize is corrected in D144773)





Peiming updated this revision to Diff 503576.Mar 8 2023, 5:18 PM
Peiming marked 6 inline comments as done.

address comments.

Peiming updated this revision to Diff 503577.Mar 8 2023, 5:22 PM

fix indention.

Peiming updated this revision to Diff 503807.Mar 9 2023, 9:19 AM

revert unintended changes.

aartbik added inline comments.Mar 10 2023, 10:14 AM

mark this with TODO or FIXME so we can easily find what we promised later ;-)


We copy


These part needs to move ABOVE examples, to L61

then, add a slide example also to doc

Peiming updated this revision to Diff 504216.Mar 10 2023, 10:43 AM
Peiming marked 4 inline comments as done.

address comments.

aartbik accepted this revision.Mar 10 2023, 10:46 AM
This revision is now accepted and ready to land.Mar 10 2023, 10:46 AM
This revision was landed with ongoing or failed builds.Mar 10 2023, 10:59 AM
This revision was automatically updated to reflect the committed changes.