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 — bpo-41323: Perform 'peephole' optimizations directly on the CFG. by markshannon · Pull Request #21517 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jul 17, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Saves about 200 lines of code
  • Breaks the coupling between the optimizer and the bytecode and line number table formats.

https://bugs.python.org/issue41323

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@markshannon
Move 'peephole' optimizations into compile.c and perform them directl…
d5b4bda 
…y on the CFG.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 17, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Jul 17, 2020
@markshannon markshannon assigned pablogsal Jul 17, 2020
@markshannon markshannon changed the title bpo-41323: Move 'peephole' optimizations directly on the CFG. bpo-41323: Perform 'peephole' optimizations directly on the CFG. Jul 17, 2020
@markshannon
Regenerate frozen importlib.
c480401
remilapeyre
remilapeyre reviewed Jul 17, 2020
View reviewed changes
Comment thread Python/compile.c Outdated
PyObject *freevars = NULL;
PyObject *cellvars = NULL;
PyObject *bytecode = NULL;
// PyObject *bytecode = NULL;

@remilapeyre remilapeyre Jul 17, 2020

Copy link
Copy Markdown
Contributor

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.

Did you forget this?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 17, 2020

Copy link
Copy Markdown
Member 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.

Yes, thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon
Remove commented out code.
6b7019a
@rhettinger

rhettinger commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

Mostly, this looks pretty good. Consider removing the peephole.c file altogether. It was originally part of compile.c and got separated out when it got too large. We can more the PyCode_Optimize() function back to compile.c. Ideally, we could drop it from the public api as part of Victor's PEP to overhaul the C API.

One small loss is that the current code has macros that provide meaningful opcode groupings like UNCONDITIONAL_JUMP, CONDITIONAL_JUMP, ABSOLUTE_JUMP, and JUMPS_ON_TRUE. I always found those grouping helpful for reasoning about the code.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Jul 17, 2020
View reviewed changes
Comment thread Python/peephole.c Outdated
Py_XINCREF(code);
PyMem_Free(blocks);
PyMem_Free(codestr);
Py_INCREF(code);

@pablogsal pablogsal Jul 17, 2020 •
edited
Loading

Uh oh!

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

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 use Py_UNUSED for the ones that are not code in the function declaration.

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 20, 2020

Copy link
Copy Markdown
Member 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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 17, 2020
@bedevere-bot

bedevere-bot commented Jul 17, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 6b7019a 🤖

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 17, 2020
pablogsal
pablogsal reviewed Jul 17, 2020
View reviewed changes
Comment thread Python/compile.c Outdated
Comment on lines 5885 to 5886
if (!names || !varnames)
goto error;

@pablogsal pablogsal Jul 17, 2020

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.

Suggested change
if (!names || !varnames)
goto error;
if (!names || !varnames) {
goto error;
}

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 20, 2020

Copy link
Copy Markdown
Member 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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed Jul 17, 2020
View reviewed changes
Comment thread Python/compile.c
optimize_cfg(struct assembler *a, PyObject *consts)
{
for (int i = 0; i < a->a_nblocks; i++) {
if (optimize_basic_block(a->a_reverse_postorder[i], consts)) {

@pablogsal pablogsal Jul 17, 2020 •
edited
Loading

Uh oh!

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

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.

Does this converge on one single iteration? What happens if you have a sequence of many jumps all together between blocks?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 20, 2020

Copy link
Copy Markdown
Member 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.

No. Some jumps that can be eliminated will still remain.

This PR is aimed at closely mimicking the behavior of the existing peephole optimizer.
We can improve it later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
pablogsal
pablogsal reviewed Jul 17, 2020
View reviewed changes

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

This looks great and very compact! This is a very good work @markshannon ! I will do a more thorough review over the weekend but I have left some minor comments for now

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed Jul 17, 2020
View reviewed changes
Comment thread Python/compile.c
for (i = 0; i < n; ++i) {
VISIT(c, expr, (expr_ty)asdl_seq_GET(s, i));
ADDOP_JABS(c, jumpi, end);
basicblock *next = compiler_new_block(c);

@pablogsal pablogsal Jul 17, 2020

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.

Huh? Why we need a new blocks here for connecting the jumps?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 20, 2020

Copy link
Copy Markdown
Member 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.

All instructions following a jump or branch must start a new basic block.

We now need the CFG generated by the code generation pass to be correct, as we perform jump elimination on it, rather than the CFG recreated in peephole.c.

Longer term, I think the front end should use labels and CFG creation should be a pass in between the code generation pass and the optimizer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon
Two minor code tidy ups.
a2915d5
@markshannon
markshannon force-pushed the move-peephole-optimizations-to-cfg branch from 7845d6f to 8551d33 Compare July 20, 2020 09:33
@markshannon

markshannon commented Jul 20, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@rhettinger
I've removed peephole.c as you suggested.

The UNCONDITIONAL_JUMP, CONDITIONAL_JUMP, ABSOLUTE_JUMP, and JUMPS_ON_TRUE don't seem that useful in an instruction based optimizer.
I initially created versions of them, but it turned out they were unused. So I deleted them.
For jump-to-jump elimination, it turns out that the exact opcode is more important that whether a jump is conditional and/or relative.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Remove peephole.c
7bc2522
@markshannon
markshannon force-pushed the move-peephole-optimizations-to-cfg branch from 8551d33 to 7bc2522 Compare July 20, 2020 09:49
@markshannon
markshannon requested a review from a team as a code owner July 20, 2020 09:49
sweeneyde
sweeneyde reviewed Jul 20, 2020
View reviewed changes
Comment thread Python/compile.c
}
}
}
}

@sweeneyde sweeneyde Jul 20, 2020

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.

Don't we need one of these?

Suggested change
}
}
PyObject_Free(stack);

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Jul 20, 2020

Copy link
Copy Markdown
Member 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.

Yes, thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon
Fix leak in mark_reachable.
fda0bed
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 20, 2020
@bedevere-bot

bedevere-bot commented Jul 20, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit fda0bed 🤖

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 20, 2020
rhettinger
rhettinger approved these changes Jul 21, 2020
View reviewed changes
@bedevere-bot bedevere-bot removed the awaiting core review label Jul 21, 2020
@bedevere-bot bedevere-bot added the awaiting merge label Jul 21, 2020
pablogsal
pablogsal approved these changes Jul 21, 2020
View reviewed changes
@markshannon
Treat jumps to empty blocks as jumps to the following block.
17541b6
@markshannon

markshannon commented Jul 21, 2020

Copy link
Copy Markdown
Member Author

One final tweak. Jumps to empty blocks that were immediately followed by jumps were not being eliminated. The final commit fixes that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Regenerate importlib
1b57a42
pablogsal
pablogsal approved these changes Jul 21, 2020
View reviewed changes
@markshannon
markshannon merged commit 6e8128f into python:master Jul 30, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 30, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020
@markshannon @shihai1991
bpo-41323: Perform 'peephole' optimizations directly on the CFG. (pyt…
2c6db7b 
…honGH-21517)

* Move 'peephole' optimizations into compile.c and perform them directly on the CFG.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
@markshannon @shihai1991
bpo-41323: Perform 'peephole' optimizations directly on the CFG. (pyt…
9b6d58d 
…honGH-21517)

* Move 'peephole' optimizations into compile.c and perform them directly on the CFG.
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@markshannon @xzy3-cdc
bpo-41323: Perform 'peephole' optimizations directly on the CFG. (pyt…
5f56f9c 
…honGH-21517)

* Move 'peephole' optimizations into compile.c and perform them directly on the CFG.
@markshannon
markshannon deleted the move-peephole-optimizations-to-cfg branch October 19, 2020 13:45
@ezio-melotti ezio-melotti mentioned this pull request Nov 14, 2022
Use :cpy-file: throughout the Devguide python/devguide#984
Merged
@ezio-melotti ezio-melotti mentioned this pull request Dec 9, 2022
Fix links to missing CPython files python/devguide#1003
Open
@ezio-melotti ezio-melotti mentioned this pull request Dec 25, 2022
Fix remaining broken links. python/devguide#1013
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sweeneyde sweeneyde sweeneyde left review comments
@rhettinger rhettinger rhettinger approved these changes
@pablogsal pablogsal pablogsal approved these changes
+1 more reviewer
@remilapeyre remilapeyre remilapeyre left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@pablogsal pablogsal

Labels

None yet

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.

7 participants

@markshannon @rhettinger @bedevere-bot @pablogsal @remilapeyre @sweeneyde @the-knights-who-say-ni
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.