dialects: (transform) Add some transform dialect operations #2973
Conversation
|
@jorendumoulin @JosseVanDelm please sort out reviewers |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2973 +/- ##
==========================================
- Coverage 89.86% 89.84% -0.02%
==========================================
Files 409 410 +1
Lines 51085 51315 +230
Branches 7923 7955 +32
==========================================
+ Hits 45905 46106 +201
- Misses 3926 3942 +16
- Partials 1254 1267 +13 ☔ View full report in Codecov by Sentry. |
xdsl/dialects/transform.py
Outdated
| # TODO: Find out how to also use the enum FailurePropagationModeAttr as well as AnyIntegerAttr | ||
|
|
||
| body = region_def("single_block") | ||
| failure_propagation_mode = prop_def(AnyIntegerAttr) |
There was a problem hiding this comment.
why does it need to be anyintegerattr? can it not just be failureproagationmodeattr?
There was a problem hiding this comment.
both IntegerAttr and FailurePropagationModeAttr are supported. for most use cases, it looks like the IntegerAttr is used so I just used this until FailurePropagationModeAttr was also completed
There was a problem hiding this comment.
Thank you @JonasCrols! This is a very cool PR!
I made a few nit comments on:
- Adding docstrings to certain ops.
- Support other inits (e.g. also support a list of integers instead of just a DenseArrayAttr).
I think some more refactoring could make that easily possible, although I don't think this is essential for this PR.
My biggest issue is that currently it is not very clear which version of the transform dialect we would be supporting.
I think this is an important discussion to have first, especially since it seems quite unstable (as you have noticed). I'm curious what you and others think about this.
I think it would be nice/important(?) if we could also add transform interop tests to this PR.
| @@ -1,13 +1,49 @@ | |||
| // RUN: XDSL_ROUNDTRIP | |||
There was a problem hiding this comment.
I'm not sure which version of MLIR we are supporting now.
Should we not also include interoperability tests? Or maybe this is okay for a next PR?
@JonasCrols let me know if you need help in adding the interoperability test!
(Essentially you would be "round-tripping" between xdsl-opt and mlir-opt, instead of just xdsl-opt and xdsl-opt)
There was a problem hiding this comment.
It's in the readme, I would say that we want to match the syntax 1:1 with the version of MLIR in that commit, and not the latest MLIR, and if there's a reason to update the MLIR version then we can do that also.
tests/filecheck/mlir-conversion/with-mlir/dialects/transform/transform_types.mlir
Show resolved
Hide resolved
|
Please reach out on the Zulip if you need any help with the tests |
xdsl/dialects/transform.py
Outdated
|
|
||
| body = region_def("single_block") | ||
| failure_propagation_mode: Property = prop_def( | ||
| T # pyright: ignore[reportArgumentType] |
There was a problem hiding this comment.
please remove this pyright ignore
There was a problem hiding this comment.
I'm currently trying to refactor some things for the newer Pyright version to be happy, but for the time being please use make pyright as the source of truth
tests/filecheck/mlir-conversion/with-mlir/dialects/transform/transform_types.mlir
Outdated
Show resolved
Hide resolved
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
We'll be using this to automatically tile all matmul operations. As of now, our plan is to use the mlir |
Adds 7 operations of the transform dialect (SequenceOp, YieldOp, TileOp, TileToForallOp, SelectOp, NamedSequenceOp, CastOp), as well as 4 new types (OperationType, ParamType, TypeParamType,TransformHandleType). All with tests and filecheck