This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] deduplicate non-unique coordinates when coiterating COO tensors
ClosedPublic

Authored by Peiming on Mar 7 2023, 10:47 AM.

Diff Detail

Event Timeline

Peiming created this revision.Mar 7 2023, 10:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Mar 7 2023, 10:47 AM
Peiming updated this revision to Diff 503097.Mar 7 2023, 10:49 AM

fix typo.

aartbik added inline comments.Mar 7 2023, 10:52 AM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
133

De-duplicates?

585

const? also, just spell out the type?
{I can never remember if we prefer auto in the simple or in the hard cases ;-)

763

typo: level

791

same?

917

fast forward (no s)

mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h
193

a non-unique (no an)
to fast forward (missing r)

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1453

maybe move this TODO:sort into the loop emitter, since it is probably going to be inserted there?

Peiming updated this revision to Diff 503098.Mar 7 2023, 10:52 AM

revert outdated change

Peiming added inline comments.Mar 7 2023, 10:54 AM
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
585

You are right, I reverted it. (It is left from some experimental change that I did not fully revert)

Peiming updated this revision to Diff 503101.Mar 7 2023, 10:57 AM
Peiming marked 7 inline comments as done.

address comments.

Peiming updated this revision to Diff 503103.Mar 7 2023, 11:02 AM

fix memory leak

Peiming added inline comments.
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_coo_test.mlir
175

Why do I need to free the dense return here? @springerm

aartbik accepted this revision.Mar 7 2023, 12:00 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp
765

it almost feels you can do this unconditionally, since it won't be set if not unique
but perhaps not worth it ;-)

This revision is now accepted and ready to land.Mar 7 2023, 12:00 PM
This revision was automatically updated to reflect the committed changes.
Peiming marked an inline comment as done.