- 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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? | |
739 | Nit: spurious empty line? | |
2326 | 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 | ||
121–122 | Can you rename this getTopOfStack? It'll be more clear for the callers. (feel free to do it in a NFC commit separately) |
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? |
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. |
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.
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 |
I don't quite get the C++ analogy here?