Page MenuHomePhabricator

[mlir][sparse] Moving the sort from factory method to the constructor.
ClosedPublic

Authored by wrengr on Jan 12 2022, 3:31 PM.

Details

Summary

This guarantees the preconditions of fromCOO; whereas prior to this, one could call the constructor directly with an unsorted tensor, which would cause fromCOO to misbehave.

Diff Detail

Event Timeline

wrengr created this revision.Jan 12 2022, 3:31 PM
wrengr requested review of this revision.Jan 12 2022, 3:31 PM
aartbik accepted this revision.Jan 13 2022, 10:09 AM

At one point the idea was the perhaps some paths do not need a sort, since it is already sorted at construction time.
In the long run, perhaps we should add a "isSorted" field to the tensor and skip sorting when already done.

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
274

sort lexicographically

This revision is now accepted and ready to land.Jan 13 2022, 10:09 AM
wrengr updated this revision to Diff 399749.Jan 13 2022, 12:10 PM
wrengr marked an inline comment as done.

addressing comments

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
274

I fleshed out the comment a bit more, but imo it's redundant to say lexicographic since that's always what SparseTensorCOO::sort does. If we really need that to be explicit everywhere, then we should rename the method to lexicographicSort

aartbik added inline comments.Jan 13 2022, 1:22 PM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
274

It was more that the originally comment was dropped. The new version seems way to verbose, how about going back to the original:

// We sort the tensor lexicographically here, to ensure the preconditions for `fromCOO`.
wrengr marked an inline comment as done.Jan 13 2022, 5:46 PM
wrengr updated this revision to Diff 399857.Jan 13 2022, 5:47 PM

addressing nits