Page MenuHomePhabricator

[mlir][Python] Roll up of python API fixes.
ClosedPublic

Authored by stellaraccident on Jan 24 2021, 2:52 PM.

Details

Summary
  • As discussed, fixes the ordering or (operands, results) -> (results, operands) in various create like methods.
  • Fixes a syntax error in an ODS accessor method.
  • Removes the linalg example in favor of a test case that exercises the same.
  • Fixes FuncOp visibility to properly use None instead of the empty string and defaults it to None.
  • Implements what was documented for requiring that trailing init args loc and ip are keyword only.
  • Adds a check to InsertionPoint.insert so that if attempting to insert past the terminator, an exception is raised telling you what to do instead. Previously, this would crash downstream (i.e. when trying to print the resultant module).
  • Renames _ods_build_default -> build_generic and documents it.
  • Removes result from the list of prohibited words and for single-result ops, defaults to naming the result result, thereby matching expectations and what is already implemented on the base class.
  • This was intended to be a relatively small set of changes to be inlined with the broader support for ODS generating the most specific builder, but it spidered out once actually testing various combinations, so rolling up separately.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Jan 24 2021, 2:52 PM
mehdi_amini accepted this revision.Jan 24 2021, 4:17 PM
mehdi_amini added inline comments.
mlir/lib/Bindings/Python/IRModules.cpp
1369

Nice error message! I love it when we can suggest a corrective alternative to users :)

mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp
646

What is the named star arguments used for here? Can you document this here? (and maybe somewhere else, in the .md doc?)

This revision is now accepted and ready to land.Jan 24 2021, 4:17 PM
mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp
646

It's just python syntax for terminating the positional argument part of the declaration. It's been around for a long time but the long Python2 phase-out means that a lot of people are just discovering that it is a thing: https://www.python.org/dev/peps/pep-3102/

Given that this is just a language feature, I'm inclined to not comment further. The doc (md file) do call this out as keyword only arguments.

This revision was landed with ongoing or failed builds.Jan 24 2021, 7:04 PM
This revision was automatically updated to reflect the committed changes.

Nice, thanks! (couple of questions, apologies if already discussed)

mlir/docs/Bindings/Python.md
457

Q: how frequent is this used via this vs via InsertionPoint? I was wondering if we called this insertion_point where we'd fall on the verbosity/self-documenting spectrum? (it may be bias but loc == location is a pretty standard abbreviation while ip is a bit new for me)

mlir/test/Bindings/Python/dialects/linalg.py
25

So the insertion point is at the end of the block and so all insertions are at end of block? (I was just thinking: what would it mean if we had an existing block and we set the position to the start of that block? If every instruction gets the same insertion point, would it result in an inversion in the ir of the ops inserted?). Being nitty here :) But just trying to think of what the insertion point context means here, C++ side I understand you set the position and then all additions follow post, so I have a mental model of a linear list of ops being, while in this interface it almost feels like I have a "fixed" insertion point used by all creates.

27

OOC: Would it be possible to not require .result? E.g., implicit cast from op to result if op has single result? Or like op[n] is nth output? (where nth == ODS returns, so could be variadic too). Or would that be more something layered on top of this?

mlir/docs/Bindings/Python.md
457

I've found that there are two styles of use of these APIs:

  • Humans writing scripts to construct IR: Tends to use context managers with implicit context, location and insertion point a lot (most of our tests are written in this style).
  • Python and/or DSL compilers that are constructing IR from some form of AST: tend to use explicit loc and ip because it is better to be explicit and not have spooky action at a distance things going on. Npcomp uses this style a lot, and ip is quite frequent. It is a lot less typing and long line wrapping to abbreviate it in those contexts (which tend to have a lot of long lines anyway).
mlir/test/Bindings/Python/dialects/linalg.py
25

I went back and forth on this a bunch when trying to come up with what we have, but happy to be nitty about it.

In this API, which follows the C-API convention, the InsertionPoint always inserts before a reference operation, with a None interpreted as the operation one-past-the-end of the Block. When constructing it you have these options:

  • InsertionPoint(ref_op): Insert before the given ref op (interpreting None as insert at end of block).
  • InsertionPoint.at_block_begin(block): If the block is not empty, same as InsertionPoint(block.operations[0]), otherwise, same as InsertionPoint(None).
  • InsertionPoint.at_block_terminator(block): Inserts before the terminator of the block, raising an error if the block is empty or if the last operation is not a known terminator.

See the insertion_point.py test for some samples of the different orderings.

I think this is actually how the C++ side OpBuilder::InsertPoint functions as well, no? The difference is that since it uses a C++ iterator, it is directly expressible to represent the operation-one-past-the-last, whereas we represent that as None on the Python side.

27

Yes, I think I've mentioned this before but it was before most people could see/use e2e examples enough to have an opinion. Not having this implicit "cast" is probably the single biggest source of typos I made in npcomp. Given that Python typing is already... loose... I wanted to have a pretty good body of work before loosening it further in that way. But I think we are there now, and a cast extension on the Operation.create related functions could make this just work.