Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-45773: Stop "optimizing" certain jump patterns #29505

Merged
merged 6 commits into from
Nov 11, 2021

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Nov 9, 2021

@brandtbucher brandtbucher added type-bug An unexpected behavior, bug, or error needs backport to 3.10 only security fixes labels Nov 9, 2021
@brandtbucher brandtbucher changed the title bpo-43773: Stop "optimizing" certain jump patterns bpo-45773: Stop "optimizing" certain jump patterns Nov 9, 2021
@markshannon markshannon self-assigned this Nov 10, 2021
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

I'm not sure about the comments // It's okay if inst->i_target == target->i_target., or the asserts assert(inst->i_target != target->i_target);

I'm not saying that they are incorrect, but it is not obvious why they are true.
Could you elaborate?

Something else to consider is to create a jump_thread function which gets all the checks if one place (different targets, compatible line numbers). It should return 1 if successful.

We could then replace code from line 8253 to 8256 with i -= jump_thread(inst, target);

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

@brandtbucher
Copy link
Member Author

I figured I'd give your jump_thread suggestion a stab, and that sort of led to a refactoring of all jump elimination. I do really like the way the logic here cleans up as a result.

If you think this is outside the scope of this PR, though, I can revert the latest changes and propose them separately.

Python/compile.c Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
@markshannon
Copy link
Member

I figured I'd give your jump_thread suggestion a stab, and that sort of led to a refactoring of all jump elimination. I do really like the way the logic here cleans up as a result.

I agree, this does clean things up.

@brandtbucher brandtbucher merged commit 27b69e6 into python:main Nov 11, 2021
@miss-islington
Copy link
Contributor

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @brandtbucher, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 27b69e60daa7b191ee6bc76fb6d5fb7d793062ab 3.10

brandtbucher added a commit to brandtbucher/cpython that referenced this pull request Nov 11, 2021
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Nov 11, 2021
@bedevere-bot
Copy link

GH-29526 is a backport of this pull request to the 3.10 branch.

@brandtbucher brandtbucher deleted the peephole-hang branch November 11, 2021 19:52
thatbirdguythatuknownot added a commit to thatbirdguythatuknownot/cpython that referenced this pull request Nov 23, 2021
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
remykarem pushed a commit to remykarem/cpython that referenced this pull request Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants