This is an archive of the discontinued LLVM Phabricator instance.

[mlir][transform][sparse] introduce sparse tensor transform extension.
ClosedPublic

Authored by Peiming on Aug 3 2023, 12:39 PM.

Diff Detail

Event Timeline

Peiming created this revision.Aug 3 2023, 12:39 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Aug 3 2023, 12:39 PM
Peiming planned changes to this revision.Aug 3 2023, 12:40 PM

blaze change WIP.

nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.td
28

I was in the process of making some ill-advised suggestions on how the transform should be more composable but then I realized that this is a actual match op and not a transform.

So I will defer to @ftynse as I have not yet had time to dig into that part of the system.
Offhand, it seems you want to test this as part of a transform.match.structured region rather than as a transform (see e.g. mlir/test/Dialect/Linalg/match-ops-interpreter.mlir)

Peiming updated this revision to Diff 546998.Aug 3 2023, 2:03 PM

address comments.

Peiming marked an inline comment as done.Aug 3 2023, 2:03 PM

typo in title: instroduce

mlir/include/mlir/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.h
35

looks like this is a copy and paste error from the original file that inspired you ;-)

wrengr retitled this revision from [mlir][transform][sparse] instroduce sparse tensor transform extension. to [mlir][transform][sparse] introduce sparse tensor transform extension..Aug 3 2023, 3:28 PM
wrengr added inline comments.Aug 3 2023, 3:30 PM
mlir/include/mlir/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.td
2

should adjust this line to fit the 80-char limit

Peiming updated this revision to Diff 547030.Aug 3 2023, 3:47 PM

address comments.

Peiming marked 2 inline comments as done.Aug 3 2023, 6:09 PM
ftynse accepted this revision.Aug 4 2023, 12:47 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.td
36

Nit: this can use custom<SemiFunctionType>(type($target), type($result)) if desired so it is future-proof with potential syntax changes in other match ops.

mlir/lib/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.cpp
26

Super-nit: start error messages with a non-capital letter, these are usually appended to something.

42

It doesn't (yet?) generate anything, so there's no need to declare this. Okay to keep if there is an intention to add more ops quickly.

This revision is now accepted and ready to land.Aug 4 2023, 12:47 AM
Peiming marked 3 inline comments as done.Aug 4 2023, 9:08 AM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/TransformOps/SparseTensorTransformOps.cpp
42

Yeah, that is what we expect, I will keep it.

Peiming updated this revision to Diff 547243.Aug 4 2023, 9:09 AM
Peiming marked an inline comment as done.

address comments.

Peiming updated this revision to Diff 547257.Aug 4 2023, 9:31 AM

add bazel config.

aartbik accepted this revision.Aug 4 2023, 10:25 AM
aartbik added inline comments.
mlir/test/Integration/Dialect/SparseTensor/TransformOps/match_sparse_ops.mlir
1

Perhaps others feel strongly about this, but can we simply put this in test/Dialect/SparseTensor/TransformsOps (i.e. not as an integration test). It feels different from the end-to-end run that we define there

Peiming updated this revision to Diff 547283.Aug 4 2023, 11:21 AM

move to non-integration test.

Peiming marked an inline comment as done.Aug 4 2023, 11:22 AM
Peiming added inline comments.
mlir/test/Integration/Dialect/SparseTensor/TransformOps/match_sparse_ops.mlir
1

I think it make sense to move it out integration test.

Peiming updated this revision to Diff 547286.Aug 4 2023, 11:23 AM
Peiming marked an inline comment as done.

rebase.

This revision was landed with ongoing or failed builds.Aug 4 2023, 11:40 AM
This revision was automatically updated to reflect the committed changes.
aartbik added inline comments.Aug 4 2023, 11:51 AM
mlir/test/Integration/Dialect/SparseTensor/TransformOps/match_sparse_ops.mlir
1

Thanks, feels better to me too.

Seems cause a buildbot error, fixing it...

Seems cause a buildbot error, fixing it...

https://reviews.llvm.org/D157134