Skip to content
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

gh-126835: Move optimization of constant sequence creation from codegen to CFG #129426

Merged
merged 16 commits into from
Feb 1, 2025
Merged

gh-126835: Move optimization of constant sequence creation from codegen to CFG #129426

merged 16 commits into from
Feb 1, 2025

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Jan 29, 2025 •
edited by bedevere-app bot
Loading

@Eclips4 Eclips4 requested a review from tomasr8 January 29, 2025 08:56
@bedevere-app bedevere-app bot mentioned this pull request Jan 29, 2025
Open
@Eclips4 Eclips4 mentioned this pull request Jan 29, 2025
Draft
@Eclips4 Eclips4 marked this pull request as draft January 29, 2025 09:22
@Eclips4 Eclips4 marked this pull request as ready for review January 29, 2025 10:45
iritkatriel
iritkatriel reviewed Jan 29, 2025
View reviewed changes
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
iritkatriel
iritkatriel reviewed Jan 29, 2025
View reviewed changes
Python/flowgraph.c Outdated Show resolved Hide resolved
@Eclips4 Eclips4 requested a review from iritkatriel January 29, 2025 21:07
iritkatriel
iritkatriel reviewed Jan 29, 2025
View reviewed changes
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Show resolved Hide resolved
WolframAlph
WolframAlph reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@WolframAlph WolframAlph left a comment

Choose a reason for hiding this comment

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

Some questions, suggestions and nitpicks.

Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/flowgraph.c Show resolved Hide resolved
@Eclips4 Eclips4 requested a review from WolframAlph January 31, 2025 17:37
WolframAlph
WolframAlph approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@WolframAlph WolframAlph left a comment

Choose a reason for hiding this comment

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

Looks good to me

iritkatriel
iritkatriel reviewed Feb 1, 2025
View reviewed changes
Python/flowgraph.c Outdated Show resolved Hide resolved
iritkatriel
iritkatriel approved these changes Feb 1, 2025
View reviewed changes
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

LGTM

@Eclips4 Eclips4 enabled auto-merge (squash) February 1, 2025 10:31
@Eclips4 Eclips4 merged commit 7d0521d into python:main Feb 1, 2025
41 checks passed
@Eclips4 Eclips4 deleted the move-codegen-optimization branch February 4, 2025 17:43
@WolframAlph
Copy link
Contributor

@Eclips4 , all tests pass when I disable this optimization. Is it intended? Shouldn't we have some tests for this?

@Eclips4
Copy link
Member Author

Eclips4 commented Feb 5, 2025

@Eclips4 , all tests pass when I disable this optimization. Is it intended? Shouldn't we have some tests for this?

Yes, it's expected. There are existing tests that test this. You can see one of them in this PR, which I needed to tweak.

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
@Eclips4 @iritkatriel
@WolframAlph @srinivasreddy
pythongh-126835: Move optimization of constant sequence creation from…
8c655c7 
… codegen to CFG (python#129426)

Codegen phase has an optimization that transforms
```
LOAD_CONST x
LOAD_CONST y
LOAD_CONXT z
BUILD_LIST/BUILD_SET (3)
```
->
```
BUILD_LIST/BUILD_SET (0)
LOAD_CONST (x, y, z)
LIST_EXTEND/SET_UPDATE 1
```
This optimization has now been moved to CFG phase to make python#128802 work.


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

@WolframAlph WolframAlph WolframAlph approved these changes

@iritkatriel iritkatriel iritkatriel approved these changes

@tomasr8 tomasr8 Awaiting requested review from tomasr8

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

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
@Eclips4 @WolframAlph @iritkatriel