Thanks! I think this makes sense to me.

I think this looks good to me. Not sure why the pre-merge tests failed...

Thanks for the reviews!

Some clean ups suggested in review.

I think this change may be trivial enough to land without pre-merge approval. I've been testing it in a downstream project and it seems to work as expected.

I'm not sure who to tag on Python API additions these days, maybe @mehdi_amini, @jpienaar, or @jdd could take a look?

Rebase main after C API landed.

Update test to use the new C API.

Switch to the new C API for OpOperand.

Switch to the usual DEFINE_C_API_STRUCT for MlirOpOperand.

Fixup unrelated whitespace change.

Not that it is super relevant to this patch but I'm curious: is that some kind of debug build? 379MiB seems quite hefty otherwise! I expect that maybe you are <10MiB in a stripped/release build?

Sorry for the delay, but I finally got around to taking this for a spin.

I took this for a spin in CIRCT, and we have already used the extension mechanism to write our own constructors in a lot of places this could break APIs. There was one CIRCT helper, used in two places, that required minor changes at the call sites. I just wrote up a patch since it was so simple: https://github.com/llvm/circt/pull/3134.

Ok, I manually made the change in the inline comment and that fixed my issue, so I think this should be good to go after that.

I confirmed I don't hit this issue in the same environment with the new simple python detection script disabled... I will try to do a little deep dive into this and see if I can figure out what's going on.

I do see the script correctly sets this:

Actually, may have spoken too soon. I was able to run CMake successfully, but when I went to compile the Python dynamic shared object, there were some issues finding Python.h like this:

Alright, I got a chance to test this, and I think it's working well.

This looks good to me in general. I tried taking it for a spin, but I realized the dev machine I was on didn't have the proper python environment to test this. I will check again after installing a suitable python.

Apologies for the delay... I said I'd take a look at this from CIRCT's perspective a week ago. Going this route in general sounds good to me, and the current implementation seems to make sense. I'm happy to update CIRCT as needed.

Hi @frgossen, I just incorporated this change downstream into CIRCT, and I found it somewhat unintuitive.

Update commit message.

Get rid of MLIR_BINDINGS_PYTHON_LOCK_VERSION and take add_mlir_python_extension from https://reviews.llvm.org/D111513.

Take two. I tested this one locally and I think it should work. Do the buildbots run after commit to main or is there anywhere I can check before committing?

Thanks Mehdi. I will take a look at that. We also had some issues downstream so I was planning to revert but you beat me to it.

In D111383#3050098, @stellaraccident wrote:

We should propagate this to downstreams (at least a couple of which already require this version of cmake and don't need the conditional).

Makes sense to me.

This has been working out of the box for the CIRCT project. No more TypeID suprises, etc. Thanks Stella, this is super nice!

In D106520#2898374, @stellaraccident wrote:

If you'd like to wait until tomorrow, I'll have an npcomp branch staged with changes and you should be able to draft onto those. Probably easier than being the first to apply in a new project.

I've reviewed, and I think I'm understanding what's going on. I don't think I have enough CMake chops to provide serious suggestions/alternatives on the approach, so I'm just going to try it out with CIRCT and see how it goes. Assuming it works and we don't have to work around TypeID issues and whatnot, I'm satisfied!

I haven't reviewed yet, but it's great to see this. I'll dig in today, and I'll help integrate the changes into CIRCT. The CI builds of CIRCT use the github.com/circt/llvm repo, and we can stage this there for testing as needed.

Makes sense to me.

Thanks for pulling this upstream. It looks good to me, but I'm just a user, not a pybind expert.

Thanks for the assistance @stellaraccident and @mehdi_amini!

Add TODO about invalidating children of an erased operation.

Oops, I see your comment. I will leave a TODO for now :)

Also, when considering trees of operations, it is rather hard to make these kind of mutations 100% memory safe. We had discussed some approaches, but as it stands, if you erase a tree of ops and have a deep reference into the tree you just deleted, you're going to have a bad time. The expensive solution is, prior to erasing, iterate over children recursively, see if they have a corresponding PyOperation and invalidate it. I had toyed with other approaches to optimize that but never implemented them (i.e. if every operation had a monotonically increasing number associated with it, you could do some cheaper things and not run afoul of pointer re-use issues).

Implement Stella's feedback.

Thanks @stellaraccident, I had missed that we can use the valid bit for this. I will give it a shot.

I've updated the test case to show what I mean. The bottom line is I need to make sure the Python reference is dropped and liveOperations is cleaned out before I can create any more operations. In the test, the addition of Operation.create("custom.op2") would hit the previously linked assertion, unless I added the del op.

Update test case to show previous issue.

This simple case works, but the previous concern about having the erased operation's pointer in the live operations map is a real issue. In fact, if I try to create any operations after erasing one, I hit this assertion: https://github.com/llvm/llvm-project/blob/86f291ebb2dfc5a93e1ee83b2d5965dcfa5d8bb1/mlir/lib/Bindings/Python/IRCore.cpp#L805. I haven't dug this far, but I think maybe an implementation detail of MLIR means the same pointer will be re-used on the next operation created? Anyway, it seems we must remove the pointer when erase is called... I am adding a test for this and trying to address it.

PSA noted. When I mentioned this on discourse, it seemed like this and the previous change for attributes were expansions we could get some mileage out of without totally going off the deep end. Are there immediate linkage concerns with deriving PyConcreteType out of tree, or just the general concerns we have been discussing?

Rename "destroy" to "erase".

I did some more digging, and I think my confusion came from when operations are removed from the liveOperations map. This happens in the PyOperation destructor, and only there. When I was testing this before, I found that I was hitting the assertion in the destructor, because I had already called it once and removed the operation! Part of my confusion was how I previously wrote the test. I was checking the live operations map while I still had a Python reference, so the destructor hadn't been called.

Replace call to destructor with call to mlirOperationDestroy.

The backstory is we often need to create cycles in graph regions, and I have been working on something to aid this in Python. Circuit IR often looks like:

It isn't clear to me where/how was this assertions hit before?

Frankly I'm not sure if this is the best implementation. I do have a use-case where I think an explicit destroy call is needed, and to support that I think I had to make the destructor change. At that point, it seemed like I might as well just call the destructor in the new destroy method, but I'm not sure if that is some sort of anti-pattern.

Rebase main

Thanks Stella!

@mehdi_amini since you approved https://reviews.llvm.org/D101398, do you mind taking a look at this one as well?

Rebase main

Add cleanup code to new C unittest.

Add C unittest for operands API

This is something @jdd and I will use in circt, and seemed generally useful upstream in mlir. However, we weren't sure about the addition of mlirOperationSetOperand to the C API. Are there deeper considerations preventing this from being added? Or is it just something that was demand-driven and has yet to be demanded? I've been testing this patch locally with some small programs for about a week and haven't found any issues with setting operands from Python via the proposed C API.