This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Folding operations that try to insert zero into an all-zero sparse tensor
ClosedPublic

Authored by Peiming on Aug 23 2022, 2:16 PM.

Details

Summary

The operations to fill zero into newly allocated sparse tensor are redundant, plus it failed
to lowering the test cases provided in the patch as well.

Diff Detail

Event Timeline

Peiming created this revision.Aug 23 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Aug 23 2022, 2:16 PM
Peiming updated this revision to Diff 454959.Aug 23 2022, 2:18 PM

format...

Peiming retitled this revision from [mlir][sparse] Folding operations that tries to insert zero into an all-zero sparse tensor to [mlir][sparse] Folding operations that try to insert zero into an all-zero sparse tensor.Aug 23 2022, 2:44 PM
bixia added inline comments.Aug 24 2022, 8:23 AM
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1803 ↗(On Diff #454959)

Not needed.

mlir/test/Dialect/SparseTensor/sparse_fill_zero.mlir
5

The function doesn't need to be enclosed inside a module.

115

The function doesn't need to be public.

Peiming updated this revision to Diff 455259.Aug 24 2022, 9:55 AM

address comments

Peiming marked 2 inline comments as done.Aug 24 2022, 9:56 AM
Peiming updated this revision to Diff 455262.Aug 24 2022, 10:02 AM

address comment

Peiming marked an inline comment as done.Aug 24 2022, 10:02 AM
Peiming updated this revision to Diff 455617.Aug 25 2022, 9:23 AM

Move folding to pre rewrite before sparsification takes place

aartbik added inline comments.Aug 25 2022, 9:27 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
138

Yes, I like this much better. Move it even before static shape test.

Peiming updated this revision to Diff 455621.Aug 25 2022, 9:30 AM

Allow folding zero-yielding generic op on dynamic sized sparse tensor.

Peiming marked an inline comment as done.Aug 25 2022, 9:31 AM
aartbik added inline comments.Aug 25 2022, 9:47 AM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
137

Maybe move this comment above L135 now and change it into

// Incorporate zero value into a dense, statically allocation copy.

so that the sparse/dense cases are more clear, and the test on L135/136 is not dangling

mlir/test/Dialect/SparseTensor/sparse_fill_zero.mlir
3

unrelated to your change, but this test shows that perhaps we should consider making this pre-rewriting its own pass, so we can test the input/output in isolation :-) :-)

But this is a good test to have regardless.

3

Nit: perhaps use DCSR instead of MAT

Peiming updated this revision to Diff 455624.Aug 25 2022, 9:52 AM
Peiming marked 3 inline comments as done.

Address comments

aartbik accepted this revision.Aug 25 2022, 9:58 AM

ship it!

This revision is now accepted and ready to land.Aug 25 2022, 9:58 AM
Peiming added inline comments.Aug 25 2022, 9:59 AM
mlir/test/Dialect/SparseTensor/sparse_fill_zero.mlir
3

Also, changed the function name to be more meaningful (instead of main).

This revision was landed with ongoing or failed builds.Aug 25 2022, 10:00 AM
This revision was automatically updated to reflect the committed changes.