This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Extend sparse_tensor.sort with a enum attribute to specify a sorting implementation.
ClosedPublic

Authored by bixia on Jan 26 2023, 5:00 PM.

Details

Summary

Currently, all the non-stable sorting algorithms are implemented via the
straightforward quick sort. This will be fixed in the following PR.

Diff Detail

Event Timeline

bixia created this revision.Jan 26 2023, 5:00 PM
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Jan 26 2023, 5:00 PM
aartbik added inline comments.Jan 27 2023, 11:21 AM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
332

I think part of the TODO should be here. Also, can you add the table you shared during the internal discussions here in ASCII format (sorting vs. stable property)

aartbik added inline comments.Jan 27 2023, 11:23 AM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
589

This is also a TODO, but I think you should put it at the attribute doc, not here

bixia updated this revision to Diff 492899.Jan 27 2023, 1:58 PM
bixia marked an inline comment as done.

Move a TODO to the enum definition. Add a table to show the 8 values for (stabl|nonstable, sort).

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
332

Move the TODO to here and add the table.

aartbik accepted this revision.Jan 27 2023, 2:20 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
332

typo: four implementations (plural)

332

typo: we only provide (no past tense)

333

typo: the the (first the on previous line)

This revision is now accepted and ready to land.Jan 27 2023, 2:20 PM
bixia updated this revision to Diff 492904.Jan 27 2023, 2:35 PM
bixia marked 3 inline comments as done.

Fix typoes.

wrengr added inline comments.Jan 27 2023, 2:37 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
337

Shouldn't have an underscore here, since that's not used when naming any of the other options

bixia updated this revision to Diff 492922.Jan 27 2023, 3:18 PM

remove a _ in a enum.