This is an archive of the discontinued LLVM Phabricator instance.

First pass on MLIR python context lifetime management.
ClosedPublic

Authored by stellaraccident on Sep 18 2020, 12:23 AM.

Details

Summary

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 12:23 AM
stellaraccident requested review of this revision.Sep 18 2020, 12:23 AM
ftynse accepted this revision.Sep 18 2020, 1:21 AM

As discussed on the forum. Thanks!

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

Nit: add a trailing dot.

209–210

Won't ASAN complain about this being a leak when we turn it on? Is there an issue with having it on stack as static?

We may also want to think about this being executed in a multithreaded context.

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

This seems to be move-constructing out of a copy (the object argument is copied).

31

Leftover debug print

86–103

This is still public, so someone could change it after construction at break the relationship between C and Python contexts.

This revision is now accepted and ready to land.Sep 18 2020, 1:21 AM
stellaraccident marked 2 inline comments as done.

Address comments and explicitly pass context refs to child objects where available (avoids a global table lookup in common cases).

Remove remaining fallback cases for the global context lookup in child object construction (and remove corresponding constructors).

I cleaned this up further by removing the fallback through the global context table for all of the "normal" IR cases (that already have access to something they can turn into a PyMlirContextRef). Then I removed the corresponding constructors and special cased the one remaining where we don't have an explicit ref (Named Attributes - which need to be cleaned up anyway). This should degenerate to just a Python IncRef on the python object for each such case and eliminate most of the accounting.

mlir/lib/Bindings/Python/IRModules.cpp
209–210

Changed back to a normal static. I originally had this as a class-level static, but that was triggering a warning we have enabled about global destructors. When I moved it to a function-level static, I preserved the hack, but it appears to not trigger the warning in that context.

As for ASAN, I'll take a TODO to update docs with how to do it properly for Python extensions: I did get it working once on this project, but it wasn't the most straight-forward thing, and I've lost the sequence I used.

Regarding threading: this is already thread safe by way of our friend the GIL. I've guarded the accesses with a py::gil_scoped_acquire, which should future proof this for when we have entry points that explicitly release the gil (i.e. long running).

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

My eyes might not be working right, but this looks correct to me.

A new reference created via (or similar):

PyMlirContextRef(&ref1.referrent, ref1.getObject())

The copy (ref-count increase) happens at the caller side and then the constructor moves it into storage (avoiding an additional copy and ref-count twiddle).

If you think of py::object as having the same semantics of std::shared_ptr, that is pretty close (with respect to ref counts).

86–103

Changed to an accessor.

This revision was landed with ongoing or failed builds.Sep 18 2020, 12:18 PM
This revision was automatically updated to reflect the committed changes.