gh-148211: decompose _INSERT_1_LOAD_CONST_INLINE(_BORROW) in JIT#148283
gh-148211: decompose _INSERT_1_LOAD_CONST_INLINE(_BORROW) in JIT#148283Fidget-Spinner merged 9 commits intopython:mainfrom
_INSERT_1_LOAD_CONST_INLINE(_BORROW) in JIT#148283Conversation
|
It seems this does not build? |
|
Think I've messed up the merge a bit, I'll get it sorted. |
|
That should be it. Tests failed with HTTP 504, I'll update branch and give it another go. |
|
Just so you know, JIT development has been very active recently (thanks to you and other contributors!). So i'm going to merge this PR after merging #144963 |
|
That means you'll probably have to merge in main again after that, thanks! |
|
It's great to see so much activity on JIT lately. Wouldn't have been possible without the reviews from you and other core devs. Cheers🥂 Happy to rebase once that's in. |
markshannon
left a comment
There was a problem hiding this comment.
It looks like every call to optimize_to_bool now passes _LOAD_CONST_INLINE_BORROW as its load_op argument.
So, drop that argument.
Likewise for the op arguments to lookup_attr, so it only needs the prefix and suffix arguments.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
markshannon
left a comment
There was a problem hiding this comment.
Looks good.
One small suggestion.
Lib/test/test_capi/test_opt.py
Outdated
| self.assertNotIn("_LOAD_ATTR_METHOD_NO_DICT", uops) | ||
| self.assertNotIn("_INSERT_1_LOAD_CONST_INLINE", uops) | ||
| self.assertIn("_INSERT_1_LOAD_CONST_INLINE_BORROW", uops) | ||
| self.assertNotIn("_INSERT_1_LOAD_CONST_INLINE_BORROW", uops) |
There was a problem hiding this comment.
Can you remove this line and the one above.
There is no point in asserting that a non-existent uop isn't present.
There was a problem hiding this comment.
You're right, bit of a brainfart when I wrote that.
|
You need to regen files after pulling in main. |
|
Oops, thanks for the remind. |
|
@markshannon I'm gonna assume that based off your last comment, this PR is fine to merge now. I think it looks good to me. |
|
@NekoAsakura all the |
Sure, I was going to leave them open in case anyone fancied having a go, but if nobody's picked them up I'm happy to crack on with it. |
Uh oh!
There was an error while loading. Please reload this page.