This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] extend unpack operation to support unpacking a batched COO type
ClosedPublic

Authored by Peiming on Apr 24 2023, 2:55 PM.

Diff Detail

Event Timeline

Peiming created this revision.Apr 24 2023, 2:55 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Apr 24 2023, 2:55 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/BufferizableOpInterfaceImpl.cpp
157

@springerm Do you know why this does not work and I still need to manually deallocate returned tensors?

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
692

This is a unrelated bug (but I think it should be trivial enough to be reviewed in the same patch). Let me know if you want it in a separate patch! @aartbik

Peiming updated this revision to Diff 516546.Apr 24 2023, 2:59 PM

remove debugging code.

Peiming added inline comments.Apr 24 2023, 3:05 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_pack.mlir
167

@springerm See here

aartbik added inline comments.Apr 24 2023, 3:08 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
130

new syntax and verification needs roundtrip.mlir and invalid.mlir coverage

150

this needs a new example

Peiming updated this revision to Diff 516557.Apr 24 2023, 3:44 PM

add roundtrip/invalid test + examples.

Peiming updated this revision to Diff 516559.Apr 24 2023, 3:45 PM

fix typo.

aartbik added inline comments.Apr 24 2023, 4:03 PM
mlir/lib/Dialect/SparseTensor/Transforms/BufferizableOpInterfaceImpl.cpp
156–161

(1) We allocate

(2) Can you keep the comment for == 0, (The "Similar to InsertOp ...."

mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
221

This "as explained above" has become a bit of a misleading link after all the code movement

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
692

Fine for now, but indeed, in the future just send such one liners out as separate revision

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
1251

this should really be in the "helper method" section above (we try to keep the rewriting rules together, and the helper methods together)

1251

same here, in principle, but since that was already here, we can leave that for a later cleanup

1437

I am okay keeping the genUnBatchedUnpackOp/genBatchedUnpackOp methods here, since they are really just the conversion rules

1480

this is a helper, so move up

Peiming updated this revision to Diff 516589.Apr 24 2023, 5:24 PM
Peiming marked 9 inline comments as done.

address comments.

aartbik accepted this revision.Apr 28 2023, 3:47 PM
This revision is now accepted and ready to land.Apr 28 2023, 3:47 PM
Peiming updated this revision to Diff 518484.May 1 2023, 10:15 AM

provide more build() implementation.