GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126835: Move constant unaryop & binop folding to CFG by WolframAlph · Pull Request #129550 · python/cpython

Conversation

@WolframAlph

@WolframAlph WolframAlph commented Feb 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Move const folding to the peephole optimizer #126835

This PR migrates:

  1. Unary ops optimizations from AST to CFG.
  2. All binary ops optimizations from AST to CFG except for string formatting. FTR: Move const folding to the peephole optimizer #126835 (comment)

cc @Eclips4 @tomasr8

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
WolframAlph requested review from Eclips4, iritkatriel, isidentical and markshannon as code owners February 1, 2025 16:27
@bedevere-app bedevere-app Bot added the awaiting review label Feb 1, 2025
@WolframAlph
WolframAlph marked this pull request as draft February 1, 2025 16:27
@bedevere-app bedevere-app Bot mentioned this pull request Feb 1, 2025
Move const folding to the peephole optimizer #126835
Open
@bedevere-app bedevere-app Bot removed the awaiting review label Feb 1, 2025
@Eclips4 Eclips4 added the skip news label Feb 8, 2025
WolframAlph added 2 commits February 14, 2025 22:53
@WolframAlph
move binop folding to cfg
04b4a84
@WolframAlph
update tests
a6babab
@WolframAlph
WolframAlph force-pushed the fold-binop-cfg branch from 7140ad4 to a6babab Compare February 14, 2025 21:53
WolframAlph added 3 commits February 14, 2025 23:46
@WolframAlph
add match case folding tests
0fa7c4c
@WolframAlph
add peepholer tests
27b1a09
@WolframAlph
polish ast code
d3df8c8
@WolframAlph WolframAlph changed the title gh-126835: [WIP]: move binop folding to cfg gh-126835: Move constant binop folding to CFG Feb 15, 2025
@WolframAlph
WolframAlph marked this pull request as ready for review February 15, 2025 08:28
@bedevere-app bedevere-app Bot added the awaiting review label Feb 15, 2025
@WolframAlph
add assertions
e82841a
tomasr8
tomasr8 reviewed Feb 15, 2025
View reviewed changes
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_ast/test_ast.py Outdated
self.assert_ast(result_code, non_optimized_target, optimized_target)

def test_folding_match_case_allowed_expressions(self):
source = textwrap.dedent("""

@tomasr8 tomasr8 Feb 15, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do we also want to check for invalid expressions like duplicate mapping keys? For example:

match x:
    case {0j: y, 0j: z} ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@WolframAlph WolframAlph Feb 15, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

They won't trigger error here, this is only ast parsing phase. Error is emitted in codegen.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomasr8 tomasr8 Feb 15, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

yeah but it's not tested anywhere currently and since we're changing the logic around match, it feels safer to make sure we don't change something accidentally. Though I don't insist on adding it in this PR, it's more of a general comment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@WolframAlph WolframAlph Feb 15, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We should test that, but I am not sure if in this PR as it only touches AST related logic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iritkatriel iritkatriel Feb 15, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can add tests covering various aspects of the feature you're touching.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
use IS_NUMERIC_CONST_EXPR
cd6fbd7
tomasr8
tomasr8 reviewed Feb 15, 2025
View reviewed changes
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
WolframAlph
WolframAlph commented Feb 15, 2025
View reviewed changes
Comment thread Lib/test/test_peepholer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
WolframAlph added 7 commits February 15, 2025 20:05
@WolframAlph
bring back tests
300976e
@WolframAlph
move unaryop to cfg
3240d8e
@WolframAlph
add tests
74d2275
@WolframAlph
cancel out unary not
8358e28
@WolframAlph
fix optimize_if_const_unaryop
c1a1be5
@WolframAlph
add test_optimize_unary_not
ef9221a
@WolframAlph
add optimize_unary_not_non_const
598adce
@WolframAlph
WolframAlph requested a review from iritkatriel February 20, 2025 14:22
@WolframAlph

WolframAlph commented Feb 20, 2025

Copy link
Copy Markdown
Contributor Author

@iritkatriel CI reports unused functions, but we need them in assertions. So either mark them as used or convert them to macros I guess?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel

iritkatriel commented Feb 20, 2025

Copy link
Copy Markdown
Member

@iritkatriel CI reports unused functions, but we need them in assertions. So either mark them as used or convert them to macros I guess?

Put them under #ifndef NDEBUG.
WolframAlph reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@WolframAlph
define is_unarynegative_const_complex_expr & is_allowed_match_case_bi…
099afba 
…nary_expr only on debug builds
iritkatriel
iritkatriel reviewed Feb 20, 2025
View reviewed changes
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@WolframAlph
try to fold match case expression without checks
0dad7c1
WolframAlph
WolframAlph commented Feb 20, 2025
View reviewed changes
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
WolframAlph added 2 commits February 20, 2025 21:44
@WolframAlph
simplify folding
1e8b552
@WolframAlph
address review
f4e9a42
@WolframAlph

WolframAlph commented Feb 20, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I've addressed review & simplifed match pattern folding. One question remaining whether we should add tests with hand crafted ast with invalid expressions in match statement case.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel reviewed Feb 20, 2025
View reviewed changes

@iritkatriel iritkatriel left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/ast_opt.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel

iritkatriel commented Feb 20, 2025

Copy link
Copy Markdown
Member

This is looking good. I made some final minor comments.

Are you still planning to add tests for invalid hand-crafted ASTs?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@WolframAlph
minor adjustments
2dba23f
@WolframAlph

WolframAlph commented Feb 21, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Done. Honestly I don't think we need them. We already test folding in match cases quite extensively, and current folding is the same as previous, just for limited expressions. We could add them for completeness, but probably in another PR as this one is getting quite big. But if you think we need them, I could add to this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel

iritkatriel commented Feb 21, 2025

Copy link
Copy Markdown
Member

Sure.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 21, 2025
iritkatriel
iritkatriel approved these changes Feb 21, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 21, 2025
@iritkatriel

iritkatriel commented Feb 21, 2025

Copy link
Copy Markdown
Member

It would be good to add smoke tests to make sure we don't crash on invalid ast input. Can be in another pr.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@WolframAlph

WolframAlph commented Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @iritkatriel for guidance & patience.
iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
Merge branch 'main' into fold-binop-cfg
cafbc61
@iritkatriel iritkatriel added 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section and removed 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Feb 21, 2025
@iritkatriel

iritkatriel commented Feb 21, 2025

Copy link
Copy Markdown
Member

!buildbot refleak
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 21, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit cafbc61 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F129550%2Fmerge

The command will test the builders whose names match following regular expression: refleak

The builders matched are:

  • aarch64 RHEL8 Refleaks PR
  • s390x RHEL8 Refleaks PR
  • AMD64 RHEL8 Refleaks PR
  • AMD64 FreeBSD Refleaks PR
  • PPC64LE Fedora Stable Refleaks PR
  • PPC64LE CentOS9 Refleaks PR
  • AMD64 CentOS9 NoGIL Refleaks PR
  • AMD64 Fedora Stable Refleaks PR
  • AMD64 Fedora Rawhide NoGIL refleaks PR
  • aarch64 Fedora Stable Refleaks PR
  • PPC64LE Fedora Rawhide Refleaks PR
  • AMD64 CentOS9 Refleaks PR
  • PPC64LE RHEL8 Refleaks PR
  • s390x RHEL9 Refleaks PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • AMD64 Windows11 Refleaks PR
  • PPC64LE Fedora Rawhide NoGIL refleaks PR
  • AMD64 Fedora Rawhide Refleaks PR
  • aarch64 Fedora Rawhide Refleaks PR
  • aarch64 CentOS9 Refleaks PR
  • aarch64 Fedora Rawhide NoGIL refleaks PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
iritkatriel merged commit 38642bf into python:main Feb 21, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 21, 2025
This was referenced Mar 26, 2026
Performance regression: O(N²) compile time for large constant tuples after const folding moved to CFG #146455
Closed
gh-146455: Fix O(N²) in add_const() after constant folding moved to CFG #146456
Merged
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
@isidentical isidentical Awaiting requested review from isidentical
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@Eclips4 Eclips4 Awaiting requested review from Eclips4 Eclips4 is a code owner
@tomasr8 tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@WolframAlph @iritkatriel @bedevere-bot @tomasr8 @Eclips4
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.