This change adds full python bindings for PDL, including types and operations
with additional mixins to make operation construction more similar to the PDL
syntax.
Details
- Reviewers
ftynse - Commits
- rGed21c9276a4c: [mlir] Introduce Python bindings for the PDL dialect
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Let's put return types as leading arguments in all constructors. While I understand the motivation to make the API look like the pretty syntax of the ops themselves, this contradicts the overall design of op constructors where return types almost always come first, in both C++ and Python, and doesn't really make the syntax as close. If we want a "DSL-like" layer that looks closer to the dialect, let's have it a separate layer on top of the normal builder API and leave the latter self-consistent.
mlir/lib/Bindings/Python/DialectPDL.cpp | ||
---|---|---|
25 | Nit: add some vertical whitespace between blocks please. | |
mlir/python/CMakeLists.txt | ||
133 | I don't see this file, did you forget to git add? | |
mlir/python/mlir/dialects/PDLOps.td | ||
2 | Nit: TosaOps -> PDLOps | |
mlir/python/mlir/dialects/_pdl_ops_ext.py | ||
16–49 | Please add one-line doc comments for these. | |
52–63 | We have get_op_results_or_values in ods_common.py. | |
205–206 | I would consider using kwargs to differentiate here. ReplaceOp(op, None, [other_op, other_op]) and ReplaceOp(op, other_op) aren't that different. ReplaceOp(op, with_op=other_op), ReplaceOp(op, with_values=[other_op, other_op]) read better. | |
284–285 | This argument order inversion looks misleading (the parent constructor takes the things in the opposite order, and the vast majority of constructors take the result type as the leading argument), especially as both arguments accept an instance of Type. I would also consider kwargs here to avoid the footgun. | |
298–299 | Same here. |
mlir/python/CMakeLists.txt | ||
---|---|---|
133 | Yep, it was omitted by accident. | |
mlir/python/mlir/dialects/_pdl_ops_ext.py | ||
52–63 | One issue I've had with get_op_results_or_values is that it doesn't quite have the same signature to the one I wanted. It only accepts Operation if it's the only argument. Here, we accept a Sequence[Operation], since it was necessary to get snippets like the following example working: pattern = PatternOp(1, "rewrite_add_body") with InsertionPoint(pattern.body): ty1 = TypeOp(IntegerType.get_signless(32)) ty2 = TypeOp() root = OperationOp(types=[ty1, ty2]) rewrite = RewriteOp(root) with InsertionPoint(rewrite.add_body()): ty3 = TypeOp() newOp = OperationOp(name="foo.op", types=[ty1, ty3]) ReplaceOp(root, newOp) Here for example root is defined by passing two Operation-s as arguments, which would have been much more verbose with get_op_results_or_values since you would have had to use [ty1.results[0], ty2.results[0]]. What would be the best way to address this? | |
284–285 | The reason I've put resultType as the last argument pretty much everywhere is that in PDL it's always a trivial type that's obvious from the context such that (i.e., pdl.type op returns pdl.type type for example), and having to specify it at use-site is just a boilerplate that doesn't really add any information. Another way to resolve ambiguity here is to force result type after the * so that one has to spell out the name of the parameter in that case. I'm happy to revert if that's going too much into DSL land. |
mlir/python/mlir/dialects/_pdl_ops_ext.py | ||
---|---|---|
52–63 | get_op_results_or_values doesn't accept Sequence[Operation] because it is unclear what to do if the operations have more than a single result (or zero results). I would rather have the caller flatten the result lists to they understand what is happening. It should be possible to extend get_op_results_or_values to support Sequence[Operation] by asserting that all operations have exactly one result. get_op_result_or_value does that already. | |
284–285 | If it cannot be anything else than a specific type, just construct it directly without ever taking it from the user. Normally, even the generated constructors shouldn't have it, there may be something missing in the ODS infra that handles that. |
The change is ready for another round of review.
mlir/python/mlir/dialects/_pdl_ops_ext.py | ||
---|---|---|
284–285 | I've removed resultType whenever it's trivial, and moved it to a first position where it's not. |
Nit: add some vertical whitespace between blocks please.