Skip to content

gh-114083: apply optimization of LOAD_CONST instructions to the whole CFG before optimize_basic_block. #114408

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

Merged
merged 7 commits into from
Jan 22, 2024

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jan 21, 2024

Fixes #114083.

Currently, folding LOAD_CONST + TO_BOOL + CONDITIONAL_JUMP into an unconditional JUMP occurs in the same pass as jump threading. For this code:

Block1:
    JUMP Block2

Block2:
   LOAD_CONST x
   TO_BOOL
   CONDITIONAL_JUMP Block3

Block3:
    ...

We try to thread the jump in block1 before we collapse block2 into an unconditional jump, so threading is not applied, and we can end up with a redundant jump.

This PR splits the folding of LOAD_CONST + the following instruction into a separate pass, before the pass that applies jump threading.

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.

Looks good.
There are a couple of things that could do with minor clarifications.

@@ -1479,16 +1493,12 @@ apply_static_swaps(basicblock *block, int i)
}

static int
optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts)
basicblock_fold_load_const(PyObject *const_cache, basicblock *bb, PyObject *consts)
Copy link
Member

Choose a reason for hiding this comment

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

"fold" has many meanings in CS.
Maybe fold_jumps?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not only jumps. This function also fold LOAD_CONST + POP_TOP/RETURN_VALUE/TO_BOOL etc

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll call it optimize_load_const instead of fold_load_const.

@bedevere-app
Copy link

bedevere-app bot commented Jan 22, 2024

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

@iritkatriel iritkatriel changed the title gh-114083: apply folding of LOAD_CONST instructions to the whole CFG before optimize_basic_block. gh-114083: apply optimization of LOAD_CONST instructions to the whole CFG before optimize_basic_block. Jan 22, 2024
@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

@iritkatriel iritkatriel merged commit ed30a3c into python:main Jan 22, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python/flowgraph.c:497: _Bool no_redundant_jumps(cfg_builder *): Assertion `0' failed.
3 participants