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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Lib/test/test_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,6 +995,20 @@ def continue_in_while():
self.assertEqual('RETURN_CONST', opcodes[1].opname)
self.assertEqual(None, opcodes[1].argval)

@support.cpython_only
def test_return_ifexp_const_optimization(self):
# 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
Comment on lines +1000 to +1004
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.

def return_ifexp(x):
return 1 if x else 0

opcodes = list(dis.get_instructions(return_ifexp))
for i in -2, -1:
self.assertEqual('RETURN_CONST', opcodes[i].opname)

def test_consts_in_conditionals(self):
def and_true(x):
return True and x
Expand Down
55 changes: 44 additions & 11 deletions Python/flowgraph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1567,6 +1567,33 @@ apply_static_swaps(basicblock *block, int i)
}
}

// Attempt to get the next non-NOP instruction from the current instruction
// inst at position i in basic block bb. Return 1 if successful or 0 if
// running out of blocks.
static int
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?).

for (;;) {
if (*i < (*bb)->b_iused) {
if ((*inst)->i_opcode != NOP) {
return 1;
}
++*inst;
++*i;
}
else {
*bb = (*bb)->b_next;
if (!*bb) {
return 0;
}
*inst = (*bb)->b_instr;
*i = 0;
}
}
}

static int
basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *consts)
{
Expand All @@ -1587,7 +1614,13 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
if (opcode != LOAD_CONST) {
continue;
}
int nextop = i+1 < bb->b_iused ? bb->b_instr[i+1].i_opcode : 0;
basicblock *next_block = bb;
cfg_instr *next_inst = inst;
int next_i = i;
if (!next_instruction(&next_block, &next_inst, &next_i)) {
continue;
}
int nextop = next_inst->i_opcode;
switch(nextop) {
case POP_JUMP_IF_FALSE:
case POP_JUMP_IF_TRUE:
Expand All @@ -1605,10 +1638,10 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
INSTR_SET_OP0(inst, NOP);
int jump_if_true = nextop == POP_JUMP_IF_TRUE;
if (is_true == jump_if_true) {
bb->b_instr[i+1].i_opcode = JUMP;
next_inst->i_opcode = JUMP;
}
else {
INSTR_SET_OP0(&bb->b_instr[i + 1], NOP);
INSTR_SET_OP0(next_inst, NOP);
}
break;
}
Expand All @@ -1632,18 +1665,18 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
Py_DECREF(cnt);
break;
}
if (bb->b_iused <= i + 2) {
cfg_instr *is_instr = next_inst;
if (!next_instruction(&next_block, &next_inst, &next_i)) {
break;
}
cfg_instr *is_instr = &bb->b_instr[i + 1];
cfg_instr *jump_instr = &bb->b_instr[i + 2];
cfg_instr *jump_instr = next_inst;
// Get rid of TO_BOOL regardless:
if (jump_instr->i_opcode == TO_BOOL) {
INSTR_SET_OP0(jump_instr, NOP);
if (bb->b_iused <= i + 3) {
if (!next_instruction(&next_block, &next_inst, &next_i)) {
break;
}
jump_instr = &bb->b_instr[i + 3];
jump_instr = next_inst;
}
bool invert = is_instr->i_oparg;
if (jump_instr->i_opcode == POP_JUMP_IF_FALSE) {
Expand All @@ -1660,8 +1693,8 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
}
case RETURN_VALUE:
{
INSTR_SET_OP0(inst, NOP);
INSTR_SET_OP1(&bb->b_instr[++i], RETURN_CONST, oparg);
INSTR_SET_OP1(inst, RETURN_CONST, oparg);
INSTR_SET_OP0(next_inst, NOP);
break;
}
case TO_BOOL:
Expand All @@ -1681,7 +1714,7 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
return ERROR;
}
INSTR_SET_OP0(inst, NOP);
INSTR_SET_OP1(&bb->b_instr[i + 1], LOAD_CONST, index);
INSTR_SET_OP1(next_inst, LOAD_CONST, index);
break;
}
}
Expand Down
Loading