This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Introduce Python bindings for the PDL dialect
ClosedPublic

Authored by shabalin on Jan 17 2022, 12:49 AM.

Details

Summary

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.

Diff Detail

Event Timeline

shabalin created this revision.Jan 17 2022, 12:49 AM
shabalin requested review of this revision.Jan 17 2022, 12:49 AM
ftynse requested changes to this revision.Jan 17 2022, 1:13 AM

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.

This revision now requires changes to proceed.Jan 17 2022, 1:13 AM
shabalin added inline comments.Jan 18 2022, 2:06 AM
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.

ftynse added inline comments.Jan 18 2022, 2:49 AM
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.

shabalin updated this revision to Diff 400822.Jan 18 2022, 6:05 AM
shabalin marked 9 inline comments as done.

Address review feedback.

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.

shabalin marked an inline comment as done.Jan 18 2022, 6:06 AM
ftynse accepted this revision.Jan 18 2022, 7:10 AM
This revision is now accepted and ready to land.Jan 18 2022, 7:10 AM
This revision was landed with ongoing or failed builds.Jan 19 2022, 2:20 AM
This revision was automatically updated to reflect the committed changes.