Skip to content

GH-132732: Use pure op machinery to optimize COMPARE_OP_INT/FLOAT/STR #137062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Jul 26, 2025
Merged

GH-132732: Use pure op machinery to optimize COMPARE_OP_INT/FLOAT/STR #137062

merged 15 commits into from
Jul 26, 2025

Conversation

savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented Jul 24, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Jul 24, 2025
Open
Fidget-Spinner
Fidget-Spinner approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Awesome work! This LGTM, just a few nits for the comments.

tomasr8
tomasr8 approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

LGTM :) just one question: this also adds the _POP_TWO_LOAD_CONST_INLINE_BORROW optimization to other ops like _BINARY_OP_ADD_FLOAT and others. Is it worth to add tests for these ops as well?
Essentially something like test_compare_op_float_pop_two_load_const_inline_borrow but adapted for _BINARY_OP_ADD_FLOAT, etc.. This would mean adding quite a few tests so I'm not sure if it makes sense.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 24, 2025 •
edited
Loading

Let's leave it open for a few more days in case someone else wants to leave any comments, then merge it!

brandtbucher
brandtbucher approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

This is great! I'd love to see it extended (later) to handle even more cases.

@savannahostrowski
Copy link
Member Author

Chatted with Brandt and I think I'm going to land this PR as is. I plan to go back and do a pass to add more optimizations with _POP_TWO_LOAD_CONST_INLINE_BORROW, _POP_TOP_LOAD_CONST_INLINE_BORROW, etc. elsewhere.

@savannahostrowski savannahostrowski merged commit f7c380e into python:main Jul 26, 2025
64 checks passed
@savannahostrowski savannahostrowski mentioned this pull request Jul 26, 2025
Open
17 tasks
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@savannahostrowski @Fidget-Spinner @Agent-Hellboy
pythonGH-132732: Use pure op machinery to optimize `COMPARE_OP_INT/FL…
be20104 
…OAT/STR` (python#137062)

Co-authored-by: Ken Jin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomasr8 tomasr8 tomasr8 approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@brandtbucher brandtbucher brandtbucher approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@savannahostrowski @Fidget-Spinner @tomasr8 @brandtbucher