This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] allow foreach operation to generate out-of-order loop on non-annotated tensor.
ClosedPublic

Authored by Peiming on Feb 16 2023, 12:24 PM.

Details

Summary

No need for a temp COO and sort even when converting dense -> CSC, we can instead rotate the loop to yield a ordered coordinates at beginning.

Diff Detail

Event Timeline

Peiming created this revision.Feb 16 2023, 12:24 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Feb 16 2023, 12:24 PM
Peiming edited the summary of this revision. (Show Details)Feb 16 2023, 12:39 PM
aartbik added inline comments.Feb 16 2023, 2:14 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
1156

you will need to document the new attribute

In fact ,perhaps just add a section on

'tensor' : ....
'initArgs' : ...
'order' : ...

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

int -> unsigned, here and below

(unless that gives compile time warnings/errors)

520

period at end

aartbik accepted this revision.Feb 16 2023, 2:14 PM
This revision is now accepted and ready to land.Feb 16 2023, 2:14 PM
Peiming updated this revision to Diff 498148.Feb 16 2023, 2:50 PM
Peiming marked 3 inline comments as done.

address comments.

wrengr added inline comments.Feb 16 2023, 2:53 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
1161

Do you actually mean dimensions here? or levels?

Peiming marked an inline comment as done.Feb 16 2023, 2:55 PM
wrengr added inline comments.Feb 16 2023, 3:01 PM
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp
510

Actually, you should use Dimension for here and the next loop (and use Dimension dimRank above). Assuming I'm reading this correctly. Or if they're supposed to be levels instead, then use Level and Level lvlRank.

wrengr added inline comments.Feb 16 2023, 3:05 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
941

I'm thinking this i should be l? Then again, since coords is initialized to dimRank, that suggests this loop should really be (Level l = 0; l < lvlRank; l++) and then i should be d

Peiming updated this revision to Diff 498161.Feb 16 2023, 3:11 PM
Peiming marked 2 inline comments as done.

address comments.

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

I updated the inner loop, but the outer loop should be size_t, which equals to nnz here

Peiming updated this revision to Diff 498162.Feb 16 2023, 3:17 PM

small fixes

Peiming updated this revision to Diff 498165.Feb 16 2023, 3:22 PM

add a type alias

This revision was landed with ongoing or failed builds.Feb 16 2023, 3:23 PM
This revision was automatically updated to reflect the committed changes.
wrengr added inline comments.Feb 16 2023, 3:24 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp
677

It would be better style to use if (auto order = encDst.getDimOrdering()) here, so that you don't need to repeat that method call in the body