This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Python] Context managers for Context, InsertionPoint, Location.
ClosedPublic

Authored by stellaraccident on Oct 31 2020, 11:48 PM.

Details

Summary
  • Finishes support for Context, InsertionPoint and Location to be carried by the thread using context managers.
  • Introduces type casters and utilities so that DefaultPyMlirContext and DefaultPyLocation in method signatures does the right thing (allows explicit or gets from the thread context).
  • Extend the rules for the thread context stack to handle nesting, appropriately inheriting and clearing depending on whether the context is the same.
  • Refactors all method signatures to follow the new convention on trailing parameters for defaulting parameters (loc, ip, context). When the objects are carried in the thread context, this allows most explicit uses of these values to be elided.
  • Removes the style guide section on putting accessors to construct global objects on the PyMlirContext: this style fails to make good use of the new facility since it is often the only thing remaining needing an MlirContext.
  • Moves Module parse/creation from mlir.ir.Context to static methods on mlir.ir.Module.
  • Moves Context.create_operation to a static Operation.create method.
  • Moves Type parsing from mlir.ir.Context to static methods on mlir.ir.Type.
  • Moves Attribute parsing from mlir.ir.Context to static methods on mlir.ir.Attribute.
  • Move Location factory methods from mlir.ir.Context to static methods on mlir.ir.Location.
  • Refactors the std dialect fake "ODS" generated code to take advantage of the new scheme.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Oct 31 2020, 11:48 PM

Add error description to types.

stellaraccident edited the summary of this revision. (Show Details)Nov 1 2020, 12:02 AM
mehdi_amini accepted this revision.Nov 1 2020, 6:06 PM

Looks great! The DefaultingPyLocation (and similar for Context) with the "caster" is fitting pretty neatly here!

I only left very minor inline comments.

mlir/docs/Bindings/Python.md
143

I don't quite get the C++ analogy here?

154

Nit: as a follow up (or a pre-commit if you prefer), can you run this file through mdformat or something like this to wraps these really long paragraph?

mlir/lib/Bindings/Python/IRModules.cpp
564

Great message! I really like when we're providing user with hints to the solution instead of just shouting "missing context".

592

Can you add a comment in what is the logic you're applying here?

737

Nit: spurious empty line?

2325

Seems like this new line break shouldn't be needed?

2460

This is a nicer place for these methods compared to being on the context!

2653
mlir/lib/Bindings/Python/IRModules.h
122

Can you rename this getTopOfStack? It'll be more clear for the callers. (feel free to do it in a NFC commit separately)

This revision is now accepted and ready to land.Nov 1 2020, 6:06 PM
zhanghb97 added inline comments.Nov 1 2020, 6:34 PM
mlir/lib/Bindings/Python/IRModules.h
96

I don't get what 'k' here means (also the 'k' in the previous Docstring name), is it a special naming convention?

stellaraccident marked 10 inline comments as done.

Comments and rebase.

Looks great! The DefaultingPyLocation (and similar for Context) with the "caster" is fitting pretty neatly here!

I only left very minor inline comments.

Thanks - I was really pleased with how it turned out (once I actually got through to the end - it was a lot of untangling).

mlir/docs/Bindings/Python.md
143

I just removed that part.

Fwiw, I was referring to how when doing recursive template meta-programming in C++, you typically match and consume arguments from the left, so if setting yourself up for that, you want to prefix common arguments. Not particularly relevant to this doc, though.

154

I'll format and land a followon NFC so that the diff stays clean on this one.

mlir/lib/Bindings/Python/IRModules.cpp
564

:)

2460

Agreed - I was really happy when that worked out like this.

mlir/lib/Bindings/Python/IRModules.h
96

The 'k' prefix is a common feature of many C++ style guides, and I believe, does show up in LLVM code. However, the official recommendation is to not use it, so removed.

This revision was landed with ongoing or failed builds.Nov 1 2020, 7:02 PM
This revision was automatically updated to reflect the committed changes.

Alex, I'll be off-site away from my workstation tomorrow, so went ahead and pushed with Mehdi's review. If you have comments, feel free and I'll address in a followup.

mehdi_amini added inline comments.Nov 1 2020, 7:20 PM
mlir/lib/Bindings/Python/IRModules.h
96

It is the google style guide creeping up in upstream patches, since there is no convention in LLVM for constant I believe we never settled on any given style