Skip to content

gh-93678: apply remove_redundant_jumps in optimize_cfg - #96274

Merged
iritkatriel merged 8 commits into
python:mainfrom
iritkatriel:optimize_stage
Sep 1, 2022
Merged

gh-93678: apply remove_redundant_jumps in optimize_cfg#96274
iritkatriel merged 8 commits into
python:mainfrom
iritkatriel:optimize_stage

Conversation

@iritkatriel

@iritkatrieliritkatriel commented Aug 25, 2022

Copy link
Copy Markdown
Member

Fixes#93678.

Apply remove_redundant_jumps in optimise_cfg because it belongs there. Later in move_cold_blocks_to_end, we may need to apply it again.

Added an assertion just before assembly stage that there are no redundant jumps.

Also tidy up an unrelated error handling issue I spotted.

@iritkatrieliritkatriel added type-feature A feature request or enhancement tests Tests in the Lib/test dir skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 25, 2022
Comment threadLib/test/test_peepholer.py

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

One minor refactoring suggestion.
Otherwise, looks good.

Comment threadPython/compile.c Outdated
Comment threadPython/compile.c Outdated
@iritkatrieliritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2022
@bedevere-bot

Copy link
Copy Markdown

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2022
@iritkatriel

Copy link
Copy Markdown
MemberAuthor

One minor refactoring suggestion.
Otherwise, looks good.

So the way it is now, the Gentoo Non-Debug build fails because no_redundant_jumps is not defined:

Python/compile.c:8647:12: error: implicit declaration of function ‘no_redundant_jumps’; did you mean ‘remove_redundant_jumps’? [-Werror=implicit-function-declaration]
8647 | assert(no_redundant_jumps(g));
| ^~~~~~~~~~~~~~~~~~

If I remove the #ifdef Py_DEBUG from the definition of no_redundant_jumps then I get warnings about it not being used on my local build.

@iritkatriel

Copy link
Copy Markdown
MemberAuthor

@markshannon how about this version?

@iritkatrieliritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2022
@bedevere-bot

Copy link
Copy Markdown

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2022
@iritkatriel

Copy link
Copy Markdown
MemberAuthor

@markshannon how about this version?

Buildbots are fine with it.

@markshannon

Copy link
Copy Markdown
Member

I'd rather fix the underlying issue.
Why does the Gentoo Non-Debug build have asserts on? That doesn't sound like non-debug to me.

@markshannon

Copy link
Copy Markdown
Member

Changing the #ifdef Py_DEBUG to #ifndef NDEBUG should work, but I thought #ifdef Py_DEBUG implied #ifndef NDEBUG ande vice-versa.

Am I wrong, or the gentoo non-debug build mis-configured somehow?

@gpshead

Copy link
Copy Markdown
Member

NDEBUG can always be used independent of Py_DEBUG. (ex: we regularly build Python and the world -UNDEBUG mode at work in the default build that people test against)

@gpshead

Copy link
Copy Markdown
Member

@iritkatrieliritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 31, 2022
@bedevere-bot

Copy link
Copy Markdown

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 31, 2022
@markshannon
markshannon self-requested a review September 1, 2022 09:58
@iritkatriel
iritkatriel merged commit 894cafd into python:mainSep 1, 2022
@iritkatriel
iritkatriel deleted the optimize_stage branch October 18, 2022 14:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)skip newstestsTests in the Lib/test dirtype-featureA feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Direct unit tests for compiler optimisations

5 participants

@iritkatriel@bedevere-bot@markshannon@gpshead@mdboom