◐ Shell
reader mode source ↗
Skip to content

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

Merged
Eclips4 merged 16 commits into
python:mainfrom
Eclips4:move-codegen-optimization
Feb 1, 2025
Merged

gh-126835: Move optimization of constant sequence creation from codegen to CFG#129426
Eclips4 merged 16 commits into
python:mainfrom
Eclips4:move-codegen-optimization

Conversation

@Eclips4

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

Copy link
Copy Markdown
Member

@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
Closed
@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
@Eclips4 Eclips4 requested a review from iritkatriel January 29, 2025 21:07
WolframAlph
WolframAlph reviewed Jan 31, 2025
View reviewed changes

@WolframAlph WolframAlph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Some questions, suggestions and nitpicks.

Eclips4 and others added 2 commits January 31, 2025 18:28
@Eclips4 @WolframAlph
Apply suggestions from code review …
670478d 
Co-authored-by: Yan Yanchii <yyanchiy@gmail.com>
@Eclips4
Address Yan's review
21a21d0
@Eclips4 Eclips4 requested a review from WolframAlph January 31, 2025 17:37
WolframAlph
WolframAlph approved these changes Jan 31, 2025
View reviewed changes

@WolframAlph WolframAlph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Looks good to me

iritkatriel
iritkatriel approved these changes Feb 1, 2025
View reviewed changes

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

Hide comment

LGTM

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

WolframAlph commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

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

@Eclips4

Eclips4 commented Feb 5, 2025

Copy link
Copy Markdown
Member Author

@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 <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Yan Yanchii <yyanchiy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes

@tomasr8 tomasr8 Awaiting requested review from tomasr8

@markshannon markshannon

+1 more reviewer

@WolframAlph WolframAlph WolframAlph approved these changes

Reviewers whose approvals may not affect merge requirements

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