- User Since
- Dec 20 2019, 9:16 PM (40 w, 2 d)
Wed, Sep 23
Push just the one change.
Sun, Sep 20
Sat, Sep 19
Comments and rebase.
Fri, Sep 18
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.
Remove remaining fallback cases for the global context lookup in child object construction (and remove corresponding constructors).
Address comments and explicitly pass context refs to child objects where available (avoids a global table lookup in common cases).
Wed, Sep 16
I don't think that is related to you: that run was for a batch of changes
and the error is in a section of the tree in no way related to your change.
I think you're clear.
Thanks for the option updates. We should talk soon about enabling these by default.
Tue, Sep 15
Matches what was discussed on discourse. This will break the python bindings. Do you want to update those in this patch or have me do them in a follow-up? As a non-default feature still under development, either is in bounds; however, the changes should be small/mechanical and if you can include them, it will help.
Sun, Sep 6
Example changes to show how comments can be addressed.
Address comments and rebase.
Fri, Sep 4
Nice job on adding the PyShapedType intermediate base class -- there is some trickiness there. There is a cleaner way to get to the same end that I've left a comment on: feel free to leave a TODO or try to take it on in this patch.
Thu, Sep 3
Still just on mobile so not a comprehensive review. Thanks for working on this.
Sat, Aug 29
I am indeed out and on mobile, so just let a couple of response comments.
Aug 28 2020
Addressed comments. Thanks!
Comments and rebase.
Aug 26 2020
Aug 24 2020
Aug 23 2020
Thanks for the reviews. I've added a number of TODOs and it doesn't look like there is disagreement on the design points. Feel free to bring them up for further discussion if you feel otherwise.
Aug 21 2020
Aug 19 2020
ftynse is going offline for a time, so directing this review at Mehdi.
Aug 18 2020
I could go either way on my comment about style for returning uniqued, backing string data. Feel free to submit as-is and possibly resolve forward if there is a further discussion to be had.
Aug 17 2020
I am supportive of this cleanup (I have found this awkward in out of tree projects particularly). However, I am probably also not the right person for a detailed review compared to the existing CMake boiler-plate.
Fix lint and remove lambda decay.
Aug 16 2020
Thank you for the attempt at a move constructor, but these things need to be done a specific way and we don't want to be cloning the module. I've reverted that part of the patch and made the PyMlirModule class move-only. Also updated some comments and other non-functional things.
Refactor to proper move constructor and update comments.
Aug 14 2020
Aug 13 2020
Almost there. Once you resolve the above comments, let me know if you need me to land it.
Aug 11 2020
Aug 9 2020
Sorry for the delayed review (was offline for a few days). I've made some design-level comments but not done a full review. In general, I agree with Alex's comments and have added more specific feedback. It is quite hard to do a detailed review with all of the clang-format warnings: please make sure to format before sending the patch.
Aug 3 2020
Thank you. Updates Lgtm.