Skip to content

gh-121246: LOAD_CONST optimizer now uses a helper function to get the next non-NOP instruction possibly in a following block #121305

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

Conversation

blhsing
Copy link
Contributor

@blhsing blhsing commented Jul 3, 2024

As reported in gh-121246, the following Python code:

def func(x):
    return True if x else False

would be compiled into the following bytecode:

 2 LOAD_FAST                0 (x)
 4 POP_JUMP_IF_FALSE        2 (to 10)
 6 LOAD_CONST               1 (True)
 8 RETURN_VALUE
10 LOAD_CONST               2 (False)
12 RETURN_VALUE

The two pairs of LOAD_CONST + RETURN_VALUE are each supposed to be optimized into a RETURN_CONST but were not. In the former case, it was because a NOP was between LOAD_CONST and RETURN_VALUE as a result of a prior optimization so the peephole optmizer could not obtain RETURN_VALUE as the next instruction from LOAD_CONST. In the latter case, it was because RETURN_VALUE was in a block following that of LOAD_CONST. The current approach of using &bb->b_instr[i + 1] to obtain the next instruction would fail for both cases.

This PR made the LOAD_CONST-specific peephole optimizer obtain the next instruction instead by using a helper function that skips NOP instructions and turns to the next block if at the end of the current block.

The other optimizer functions may be able benefit from switching to the same helper function, but I decided not to make such changes to other optimizer functions unnecessarily unless there is an actual need.

…next non-NOP instruction possibly in the next block
Comment on lines +1000 to +1004
# gh-121246: Check that the CPython-specific peephole optimizer would
# properly convert pairs of LOAD_CONST + RETURN_VALUE into
# RETURN_CONST even if they are separated by NOP or in different
# blocks of instructions as a result of prior optimizations for
# returning different constants from a ternary operation
Copy link
Member

Choose a reason for hiding this comment

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

Typically here we just write "see gh-121346" in a comment. The discussion should be on the issue.

next_instruction(basicblock **bb, cfg_instr **inst, int *i)
{
++*inst;
++*i;
Copy link
Member

Choose a reason for hiding this comment

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

This function probably isn't quite right. If the current instruction is a jump then then b_next is not the next block executed. It also ignores line numbers - sometimes a NOP is there because it represents a line of code that needs to show as "executed" in tracing or coverage. We can't just skip over such NOPs (you should have a test where the ifexpr spans multiple lines).

I think rather than skip over NOPs here, we may need to add a call to remove_redundant_nops somewhere (maybe at the end of inline_small_or_no_lineno_blocks, only when changes > 0?).

@Eclips4
Copy link
Member

Eclips4 commented Oct 29, 2024

Thanks @blhsing for your efforts but there's no longer a RETURN_CONST instruction (see #125837), so I'm going to close this.

@Eclips4 Eclips4 closed this Oct 29, 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.

3 participants