Page MenuHomePhabricator

[mlir][python] Add generic operation parse APIs
ClosedPublic

Authored by rkayaith on Feb 5 2023, 3:02 PM.

Details

Summary

Currently the bindings only allow for parsing IR with a top-level
builtin.module op, since the parse APIs insert an implicit module op.
This change adds Operation.parse, which returns whatever top-level op
is actually in the source.

To simplify parsing of specific operations, OpView.parse is also
added, which handles the error checking for OpView subclasses.

Diff Detail

Event Timeline

rkayaith created this revision.Feb 5 2023, 3:02 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rkayaith updated this revision to Diff 494954.Feb 5 2023, 3:07 PM

fix comment

rkayaith published this revision for review.Feb 5 2023, 6:07 PM
mehdi_amini added inline comments.Feb 5 2023, 6:23 PM
mlir/lib/Bindings/Python/IRCore.cpp
1069

What's missing from mlir/include/mlir-c/Diagnostics.h?

rkayaith added inline comments.Feb 5 2023, 6:28 PM
mlir/lib/Bindings/Python/IRCore.cpp
1069

I'm not sure exactly what this comment refers to, but it's on all the other parse methods. Though, it would be nice if by default these methods collected errors and included them in the exception message, right now the user has to set up their own diagnostic handler.

ftynse added inline comments.Feb 7 2023, 6:14 AM
mlir/lib/Bindings/Python/IRCore.cpp
1069

It should be possible now, just nobody updated those methods after the diagnostic handling was exposed to C API. It would be great not to cargo-cult the comment and actually try implementing this.

1071

Nit: it has been possible to do throw py::value_error for quite a while now, https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html, let's not fallback to C APIs.

rkayaith updated this revision to Diff 495654.Feb 7 2023, 3:16 PM

use py::value_error, and make extra arguments keyword-only in case more need to be added

rkayaith marked an inline comment as done.Feb 7 2023, 3:17 PM
rkayaith added inline comments.
mlir/lib/Bindings/Python/IRCore.cpp
1069

I'll look into fixing these when I have some time, I had an initial question that I left here: https://github.com/llvm/llvm-project/issues/60595

rkayaith updated this revision to Diff 496714.Feb 11 2023, 1:51 PM

update bitcode -> bytecode in comments

rkayaith added inline comments.Feb 11 2023, 1:53 PM
mlir/lib/Bindings/Python/IRCore.cpp
1069

Any concerns with leaving this for a separate patch? I'd want to update all the other cases at the same time anyways, so the behaviour is consistent.

friendly ping

stellaraccident accepted this revision.Sun, Feb 26, 10:26 PM

Python changes lgtm. The parser changed make sense to but I usually defer those reviews to river.

This revision is now accepted and ready to land.Sun, Feb 26, 10:26 PM
rriddle added inline comments.Mon, Feb 27, 12:09 PM
mlir/include/mlir/Parser/Parser.h
141–146

The doc for sourceName doesn't read well. This looks like it is treated as the filename of the source buffer, can we better document this?

rkayaith updated this revision to Diff 500952.Mon, Feb 27, 3:47 PM
rkayaith marked an inline comment as done.

improve docs, put sourceName arg before sourceFileLoc since it's common to
both overloads

mlir/include/mlir/Parser/Parser.h
141–146

I added a bit more info here

ftynse accepted this revision.Wed, Mar 1, 12:28 PM
This revision was automatically updated to reflect the committed changes.