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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
138 | Yes, I like this much better. Move it even before static shape test. |
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 | ||
4 | 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. | |
4 | Nit: perhaps use DCSR instead of MAT |
mlir/test/Dialect/SparseTensor/sparse_fill_zero.mlir | ||
---|---|---|
4 | Also, changed the function name to be more meaningful (instead of main). |
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