- User Since
- Sep 16 2020, 10:22 AM (63 w, 4 d)
Oct 13 2021
Oct 12 2021
Update commit message.
Oct 11 2021
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?
Oct 7 2021
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.
Jul 29 2021
Makes sense to me.
Jul 28 2021
Jul 27 2021
This has been working out of the box for the CIRCT project. No more TypeID suprises, etc. Thanks Stella, this is super nice!
Jul 22 2021
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.
Jul 15 2021
Makes sense to me.
May 10 2021
Thanks for pulling this upstream. It looks good to me, but I'm just a user, not a pybind expert.
May 3 2021
Apr 28 2021
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.
Apr 27 2021
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.
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.
The diff shown by phab confused me at first, but I think this looks good.
Apr 23 2021
Adding @ftynse for visibility.
Apr 22 2021
I saw this, and agree this is great. I had the same question as John when I saw it. I've tested a branch of circt that included headers from this directory before, but I wasn't sure if they should move to the normal header directory eventually.
This is basically https://reviews.llvm.org/D99927 for Values instead of Operations. I've been testing it downstream in CIRCT. We have use-cases where we'd want a Python API that accepts a Value, or returns a Value, and this seemed to be missing for that.
Mar 25 2021
Looks great, thanks for adding this. I had just one question as I read through this.
Mar 8 2021
Not sure that I am qualified to accept this revision, but I do approve in general. The points in the summary make sense and sound good to me.
Feb 22 2021
Feb 21 2021
Jan 21 2021
Thanks @rriddle for the iterations on this. Looking forward to using this in CIRCT with our FunctionLike ops.
Add note that the new functionality only supports FunctionLike ops that use FunctionType for their signature.
Jan 20 2021
Fix typo in commit message.
Update title and description to match commit message.
Clean up the diff a bit.
Updates after most recent feedback.
Remove unused include.
Update revision description to match commit message.
Switch to parameterizing the pattern by op type.
Jan 19 2021
This is the patch discussed with @silvas in https://llvm.discourse.group/t/bufferization-framework-replacement/2446/7. I proceeded here along the same lines as when I wanted to generalize the eraseArguments and eraseResults helpers from FuncOp to FunctionLike, but perhaps there is a better way to achieve this. CC'ing @rriddle and @ftynse who had helpful feedback when I was working on FunctionSupport.h before.
Remove unrelated change.
Nov 3 2020
This revision was superseded by https://reviews.llvm.org/D90363.
Updates after review.
Awesome, thanks for taking a look. Those final comments all make sense.. I will clean those up.
@rriddle when you get a chance, please take another look. I think I've addressed your comments from the previous revision.
Remove unused file from previous revision.
Refactor implementations into FunctionSupport.cpp, and address other comments.
Oct 30 2020
Oct 29 2020
Moved BitVector creation into a helper in Support.