◐ Shell
reader mode source ↗
Skip to content

gh-126835: Move constant unaryop & binop folding to CFG#129550

Merged
iritkatriel merged 35 commits into
python:mainfrom
WolframAlph:fold-binop-cfg
Feb 21, 2025
Merged

gh-126835: Move constant unaryop & binop folding to CFG#129550
iritkatriel merged 35 commits into
python:mainfrom
WolframAlph:fold-binop-cfg

Conversation

@WolframAlph

@WolframAlph WolframAlph commented Feb 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@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
Open
@WolframAlph WolframAlph changed the title gh-126835: [WIP]: move binop folding to cfg Feb 15, 2025
@WolframAlph WolframAlph marked this pull request as ready for review February 15, 2025 08:28
96 hidden items Load more…
@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?

@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

WolframAlph commented Feb 20, 2025 •
edited
Loading

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.

iritkatriel
iritkatriel reviewed Feb 20, 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

Looks good!

@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?

@WolframAlph

WolframAlph commented Feb 21, 2025 •
edited
Loading

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.

@iritkatriel

iritkatriel commented Feb 21, 2025

Copy link
Copy Markdown
Member

Sure.

@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label 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.

@WolframAlph

WolframAlph commented Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @iritkatriel for guidance & patience.

@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

@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

@iritkatriel iritkatriel merged commit 38642bf into python:main Feb 21, 2025
This was referenced Mar 26, 2026
Closed
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 Eclips4 is a code owner

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

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.

5 participants

@WolframAlph @iritkatriel @bedevere-bot @tomasr8 @Eclips4