Skip to content

GH-132732: Use pure op machinery to optimize various instructions with _POP_TOP and _POP_TWO - #137577

Merged
savannahostrowski merged 21 commits into
python:mainfrom
savannahostrowski:poptimize
Sep 15, 2025
Merged

GH-132732: Use pure op machinery to optimize various instructions with _POP_TOP and _POP_TWO#137577
savannahostrowski merged 21 commits into
python:mainfrom
savannahostrowski:poptimize

Conversation

@savannahostrowski

@savannahostrowskisavannahostrowski commented Aug 9, 2025•
edited
Loading

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Aug 9, 2025
Closed
@savannahostrowskisavannahostrowski changed the title GH-132732: Use pure op machinery to optimize various _POP_TOP and _POP_TWO relevant instructionsGH-132732: Use pure op machinery to optimize various instructions with _POP_TOP and _POP_TWOAug 9, 2025
brandtbucher
brandtbucher requested changes Aug 9, 2025
View reviewed changes

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

This is super cool! Just a quick run-through for now:

Comment threadMisc/NEWS.d/next/Core_and_Builtins/2025-08-09-04-07-05.gh-issue-132732.8BiIVJ.rst Outdated
Comment threadPython/optimizer_bytecodes.c Outdated
Comment threadPython/optimizer_bytecodes.c Outdated
Comment threadPython/optimizer_cases.c.h Outdated
Comment threadPython/optimizer_cases.c.h Outdated
Comment threadTools/cases_generator/optimizer_generator.py Outdated
Comment threadPython/optimizer_bytecodes.c Outdated
Comment threadLib/test/test_capi/test_opt.py Outdated
@bedevere-app

bedevere-appBot commented Aug 9, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Fidget-Spinner
Fidget-Spinner reviewed Aug 9, 2025
View reviewed changes

@Fidget-SpinnerFidget-Spinner 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.

Very cool, just one nit.

Comment threadPython/optimizer_bytecodes.c Outdated
@savannahostrowski

savannahostrowski commented Aug 21, 2025

Copy link
Copy Markdown
MemberAuthor

@brandtbucher Do you want to take another look at this?

brandtbucher
brandtbucher approved these changes Sep 15, 2025
View reviewed changes

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

Looks good! Just a few suggestions for comments and cleaning up the generated code.

Comment threadPython/optimizer_bytecodes.c
Comment threadPython/optimizer_bytecodes.c
Comment threadPython/optimizer_cases.c.h Outdated
Comment on lines +205 to +211
if (sym_is_const(ctx, res)) {
PyObject *result = sym_get_const(ctx, res);
if (_Py_IsImmortal(result)) {
// Replace with _POP_TOP_LOAD_CONST_INLINE_BORROW since we have one input and an immortal result
REPLACE_OP(this_instr, _POP_TOP_LOAD_CONST_INLINE_BORROW, 0, (uintptr_t)result);
}
}

@brandtbucherbrandtbucherSep 15, 2025

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.

Still seeing weird indent here, maybe dedent the string in the cases generator?

Comment threadPython/optimizer_cases.c.h Outdated
@savannahostrowski
savannahostrowski merged commit 9c9a0f7 into python:mainSep 15, 2025
68 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-SpinnerFidget-SpinnerFidget-Spinner approved these changes
@brandtbucherbrandtbucherbrandtbucher approved these changes
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner
@tomasr8tomasr8Awaiting requested review from tomasr8tomasr8 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.

3 participants

@savannahostrowski@Fidget-Spinner@brandtbucher