This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Improve handling of dynamic-sizes for sparse=>dense conversion
ClosedPublic

Authored by wrengr on Oct 29 2021, 4:10 PM.

Details

Summary

Allows the result to be more dynamically-sized than the source.

Diff Detail

Event Timeline

wrengr created this revision.Oct 29 2021, 4:10 PM
wrengr requested review of this revision.Oct 29 2021, 4:10 PM
wrengr updated this revision to Diff 383525.Oct 29 2021, 4:25 PM

Rebasing

aartbik added inline comments.Oct 29 2021, 4:56 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
632

since the srcTensorTp is always more specific than the dest (but they are always congruent), passing in srcTensorTp too will actually make the dense alloc more static than it is now. Of course, you will have to cast the srcTensorTp to dstTensorTp eventually if they are not the same

perhaps not worth it, since the big savings (avoiding runtime calls) comes from your change in L627 already, and this will only make the alloc take less arguments...

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conversion_sparse2dense.mlir
133–134

you had remove this in a previous revision, but somehow it came back

wrengr updated this revision to Diff 383537.Oct 29 2021, 5:07 PM
wrengr marked an inline comment as done.

Fixing rebase errors

wrengr added inline comments.Oct 29 2021, 5:09 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
361

This change was from either D110790 or D112674, but got lost in the bad rebase

mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conversion_sparse2dense.mlir
133–134

Hrm, it's not on the perforce side; fixing the git rebase

wrengr updated this revision to Diff 383540.Oct 29 2021, 5:18 PM

Removing comment

aartbik accepted this revision.Oct 29 2021, 5:19 PM

Please remove the two lines as requested, and make sure we build green before submitting but no need for other review. LGTM

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
611–612

This seems a bit redundant, since this applies to all branches (and all ops we generate anyway).
So let's delete 611/612

632

per our offline discussion, not worth it, big savings is already done, and constant prop will do the rest

This revision is now accepted and ready to land.Oct 29 2021, 5:19 PM
wrengr marked 2 inline comments as done.Oct 29 2021, 5:21 PM