FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-148211: decompose `_INSERT_1_LOAD_CONST_INLINE(_BORROW)` in JIT by NekoAsakura · Pull Request #148283 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-148211: decompose _INSERT_1_LOAD_CONST_INLINE(_BORROW) in JIT - #148283

Merged
Fidget-Spinner merged 9 commits into
python:mainfrom
NekoAsakura:gh-148211/decompose-insert-1
Apr 9, 2026
ConversationCommits (9)ChecksFiles changed
Merged

gh-148211: decompose _INSERT_1_LOAD_CONST_INLINE(_BORROW) in JIT#148283
Fidget-Spinner merged 9 commits into
python:mainfrom
NekoAsakura:gh-148211/decompose-insert-1

Conversation

NekoAsakura commented Apr 9, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Apr 9, 2026
Closed

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

It seems this does not build?

NekoAsakura commented Apr 9, 2026

Copy link
Copy Markdown
Member Author

Think I've messed up the merge a bit, I'll get it sorted.

NekoAsakura commented Apr 9, 2026

Copy link
Copy Markdown
Member Author

That should be it. Tests failed with HTTP 504, I'll update branch and give it another go.

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

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

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

That means you'll probably have to merge in main again after that, thanks!

NekoAsakura commented Apr 9, 2026

Copy link
Copy Markdown
Member Author

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 requested changes Apr 9, 2026
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Python/optimizer_analysis.c Outdated

bedevere-app Bot commented Apr 9, 2026

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

markshannon reviewed Apr 9, 2026
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good.
One small suggestion.

Comment thread 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)

markshannon Apr 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you remove this line and the one above.
There is no point in asserting that a non-existent uop isn't present.

NekoAsakura Apr 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You're right, bit of a brainfart when I wrote that.

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

You need to regen files after pulling in main.

NekoAsakura commented Apr 9, 2026

Copy link
Copy Markdown
Member Author

Oops, thanks for the remind.

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

@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.

Fidget-Spinner merged commit 0f49232 into python:main Apr 9, 2026
86 of 87 checks passed

Fidget-Spinner commented Apr 9, 2026

Copy link
Copy Markdown
Member

@NekoAsakura all the _POP_CALL_X_LOAD_CONST_INLINE_BORROW look like an easy win (just POP_TOP, POP_TOP, LOAD_CONST_INLINE_X). Could you work on those next please. Thank you so much!

NekoAsakura deleted the gh-148211/decompose-insert-1 branch April 9, 2026 16:47

NekoAsakura commented Apr 9, 2026

Copy link
Copy Markdown
Member Author

@NekoAsakura all the _POP_CALL_X_LOAD_CONST_INLINE_BORROW look like an easy win (just POP_TOP, POP_TOP, LOAD_CONST_INLINE_X). Could you work on those next please. Thank you so much!

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.

ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
pythongh-148211: decompose _INSERT_1_LOAD_CONST_INLINE(_BORROW) in …
79174f9 
…JIT (pythonGH-148283)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

markshannon markshannon requested changes
Fidget-Spinner Fidget-Spinner approved these changes
tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
savannahostrowski Awaiting requested review from savannahostrowski

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL