This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] migrate sparse operations into new sparse tensor dialect
ClosedPublic

Authored by aartbik on Apr 28 2021, 2:18 PM.

Details

Summary

This is the very first step toward removing the glue and clutter from linalg and
replace it with proper sparse tensor types. This revision migrates the LinalgSparseOps
into SparseTensorOps of a sparse tensor dialect. This also provides a new home for
sparse tensor related transformation.

NOTE: the actual replacement with sparse tensor types (and removal of linalg glue/clutter) will follow but I am trying to keep the amount of changes per revision manageable.

Diff Detail

Event Timeline

aartbik created this revision.Apr 28 2021, 2:18 PM
aartbik requested review of this revision.Apr 28 2021, 2:18 PM
rriddle added inline comments.Apr 28 2021, 2:21 PM
mlir/include/mlir/Dialect/Sparse/IR/Sparse.h
1 ↗(On Diff #341315)

Why not SparseTensor dialect?

aartbik marked an inline comment as done.Apr 28 2021, 2:27 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/Sparse/IR/Sparse.h
1 ↗(On Diff #341315)

do you prefer that? SGMT...

rriddle added inline comments.Apr 28 2021, 2:28 PM
mlir/include/mlir/Dialect/Sparse/IR/Sparse.h
1 ↗(On Diff #341315)

Just seems like a more natural fit, given that this is supposed to be a sister of the Tensor dialect right? Or does this go beyond tensors?

aartbik marked 2 inline comments as done.Apr 28 2021, 4:12 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/Sparse/IR/Sparse.h
1 ↗(On Diff #341315)

You are absolutely right. I spend too much time referring to the stuff as just "sparse", but it really should be sparse tensors. Coming up!

aartbik retitled this revision from [mlir][sparse] migrate sparse operations into new sparse dialect to [mlir][sparse] migrate sparse operations into new sparse tensor dialect.Apr 28 2021, 5:16 PM
aartbik edited the summary of this revision. (Show Details)
aartbik updated this revision to Diff 341368.Apr 28 2021, 5:51 PM
aartbik marked an inline comment as done.

renamed sparse -> sparse tensor

bixia accepted this revision.Apr 29 2021, 8:45 AM
This revision is now accepted and ready to land.Apr 29 2021, 8:45 AM

Does Linalg need a dependency on the SparseTensor dialect? It would seem so from reading the code (or at least, some of the passes do).

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
23–24

Is there more you can add here?

Does Linalg need a dependency on the SparseTensor dialect? It would seem so from reading the code (or at least, some of the passes do).

Did I miss something? Note that the "pass" right now lives in testing only (and will migrate to SparseTensorDialect after that).

Nicolas kindly provided me a temporary home in "Linalg", but I overstayed my welcome, so it is time to get my own home ;-)

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
23–24

next CL will remove this op completely, hence the TODO
(in a nutshell, this was "glue" to bring a void pointer back to a tensor type; with the new sparse tensor type, this will be properly typed \O/)

rriddle added a comment.EditedApr 29 2021, 1:01 PM

Does Linalg need a dependency on the SparseTensor dialect? It would seem so from reading the code (or at least, some of the passes do).

Did I miss something? Note that the "pass" right now lives in testing only (and will migrate to SparseTensorDialect after that).

Nicolas kindly provided me a temporary home in "Linalg", but I overstayed my welcome, so it is time to get my own home ;-)

It seems that Linalg/Transforms/Sparsification.cpp is creating operations in the SparseTensor dialect. I don't know where that is used to know where the dependency declaration would go, I just saw the Linalg/ directory creating SparseTensor ops. If that is just in the test pass, I suppose no dependency is necessary.

Does Linalg need a dependency on the SparseTensor dialect? It would seem so from reading the code (or at least, some of the passes do).

Did I miss something? Note that the "pass" right now lives in testing only (and will migrate to SparseTensorDialect after that).

Nicolas kindly provided me a temporary home in "Linalg", but I overstayed my welcome, so it is time to get my own home ;-)

It seems that Linalg/Transforms/Sparsification.cpp is creating operations in the SparseTensor dialect. I don't know where that is used to know where the dependency declaration would go, I just saw the Linalg/ directory creating SparseTensor ops. If that is just in the test pass, I suppose no dependency is necessary.

Yes, apologies for the shuffling (you will see one ore two more...). With a new permanent home, things can finally move into the right place (and stay there ;-)

Sorry I had to revert in 086e0f05bfc2 because this broke the build with -DBUILD_SHARED_LIBS=ON

Sorry I had to revert in 086e0f05bfc2 because this broke the build with -DBUILD_SHARED_LIBS=ON

I was just constructing a fix for that ;-(

mlir/test/Dialect/SparseTensor/roundtrip.mlir