Skip to content

Commit

Permalink
pythongh-113603: Compiler no longer tries to maintain the no-empty-bl…
Browse files Browse the repository at this point in the history 
…ock invariant (python#113636)
  • Loading branch information
@iritkatriel @aisk
iritkatriel authored and aisk committed Feb 11, 2024
1 parent 994785b commit ea8bd20
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 78 deletions.
13 changes: 13 additions & 0 deletions Lib/test/test_compile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,19 @@ def test_condition_expression_with_dead_blocks_compiles(self):
# See gh-113054
compile('if (5 if 5 else T): 0', '<eval>', 'exec')

def test_condition_expression_with_redundant_comparisons_compiles(self):
# See gh-113054
compile('if 9<9<9and 9or 9:9', '<eval>', 'exec')

def test_dead_code_with_except_handler_compiles(self):
compile(textwrap.dedent("""
if None:
with CM:
x = 1
else:
x = 2
"""), '<eval>', 'exec')

def test_compile_invalid_namedexpr(self):
# gh-109351
m = ast.Module(
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed bug where a redundant NOP is not removed, causing an assertion to fail in the compiler in debug mode.
116 changes: 38 additions & 78 deletions Python/flowgraph.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,15 @@ _PyCfgBuilder_Addop(cfg_builder *g, int opcode, int oparg, location loc)
}


static basicblock *
next_nonempty_block(basicblock *b)
{
while (b && b->b_iused == 0) {
b = b->b_next;
}
return b;
}

/***** debugging helpers *****/

#ifndef NDEBUG
Expand All @@ -464,24 +473,16 @@ no_redundant_nops(cfg_builder *g) {
return true;
}

static bool
no_empty_basic_blocks(cfg_builder *g) {
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
if (b->b_iused == 0) {
return false;
}
}
return true;
}

static bool
no_redundant_jumps(cfg_builder *g) {
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
cfg_instr *last = basicblock_last_instr(b);
if (last != NULL) {
if (IS_UNCONDITIONAL_JUMP_OPCODE(last->i_opcode)) {
assert(last->i_target != b->b_next);
if (last->i_target == b->b_next) {
basicblock *next = next_nonempty_block(b->b_next);
basicblock *jump_target = next_nonempty_block(last->i_target);
assert(jump_target != next);
if (jump_target == next) {
return false;
}
}
Expand Down Expand Up @@ -961,42 +962,6 @@ mark_reachable(basicblock *entryblock) {
return SUCCESS;
}

static void
eliminate_empty_basic_blocks(cfg_builder *g) {
/* Eliminate empty blocks */
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
basicblock *next = b->b_next;
while (next && next->b_iused == 0) {
next = next->b_next;
}
b->b_next = next;
}
while(g->g_entryblock && g->g_entryblock->b_iused == 0) {
g->g_entryblock = g->g_entryblock->b_next;
}
int next_lbl = get_max_label(g->g_entryblock) + 1;
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
assert(b->b_iused > 0);
for (int i = 0; i < b->b_iused; i++) {
cfg_instr *instr = &b->b_instr[i];
if (HAS_TARGET(instr->i_opcode)) {
basicblock *target = instr->i_target;
while (target->b_iused == 0) {
target = target->b_next;
}
if (instr->i_target != target) {
if (!IS_LABEL(target->b_label)) {
target->b_label.id = next_lbl++;
}
instr->i_target = target;
instr->i_oparg = target->b_label.id;
}
assert(instr->i_target && instr->i_target->b_iused > 0);
}
}
}
}

static int
remove_redundant_nops(basicblock *bb) {
/* Remove NOPs when legal to do so. */
Expand Down Expand Up @@ -1025,10 +990,7 @@ remove_redundant_nops(basicblock *bb) {
}
}
else {
basicblock* next = bb->b_next;
while (next && next->b_iused == 0) {
next = next->b_next;
}
basicblock *next = next_nonempty_block(bb->b_next);
/* or if last instruction in BB and next BB has same line number */
if (next) {
location next_loc = NO_LOCATION;
Expand Down Expand Up @@ -1112,36 +1074,29 @@ remove_redundant_jumps(cfg_builder *g) {
* can be deleted.
*/

assert(no_empty_basic_blocks(g));

bool remove_empty_blocks = false;
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
cfg_instr *last = basicblock_last_instr(b);
assert(last != NULL);
if (last == NULL) {
continue;
}
assert(!IS_ASSEMBLER_OPCODE(last->i_opcode));
if (IS_UNCONDITIONAL_JUMP_OPCODE(last->i_opcode)) {
if (last->i_target == NULL) {
basicblock* jump_target = next_nonempty_block(last->i_target);
if (jump_target == NULL) {
PyErr_SetString(PyExc_SystemError, "jump with NULL target");
return ERROR;
}
if (last->i_target == b->b_next) {
assert(b->b_next->b_iused);
basicblock *next = next_nonempty_block(b->b_next);
if (jump_target == next) {
if (last->i_loc.lineno == NO_LOCATION.lineno) {
b->b_iused--;
if (b->b_iused == 0) {
remove_empty_blocks = true;
}
}
else {
INSTR_SET_OP0(last, NOP);
}
}
}
}
if (remove_empty_blocks) {
eliminate_empty_basic_blocks(g);
}
assert(no_empty_basic_blocks(g));
return SUCCESS;
}

Expand Down Expand Up @@ -1749,11 +1704,9 @@ optimize_cfg(cfg_builder *g, PyObject *consts, PyObject *const_cache)
{
assert(PyDict_CheckExact(const_cache));
RETURN_IF_ERROR(check_cfg(g));
eliminate_empty_basic_blocks(g);
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
RETURN_IF_ERROR(inline_small_exit_blocks(b));
}
assert(no_empty_basic_blocks(g));
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
RETURN_IF_ERROR(optimize_basic_block(const_cache, b, consts));
assert(b->b_predecessors == 0);
Expand All @@ -1768,14 +1721,21 @@ optimize_cfg(cfg_builder *g, PyObject *consts, PyObject *const_cache)
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
if (b->b_predecessors == 0) {
b->b_iused = 0;
b->b_except_handler = 0;
}
}
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
remove_redundant_nops(b);
}
eliminate_empty_basic_blocks(g);
assert(no_redundant_nops(g));
RETURN_IF_ERROR(remove_redundant_jumps(g));

for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
remove_redundant_nops(b);
}

RETURN_IF_ERROR(remove_redundant_jumps(g));

assert(no_redundant_jumps(g));
return SUCCESS;
}

Expand Down Expand Up @@ -1825,7 +1785,6 @@ insert_superinstructions(cfg_builder *g)
for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
remove_redundant_nops(b);
}
eliminate_empty_basic_blocks(g);
assert(no_redundant_nops(g));
}

Expand Down Expand Up @@ -2299,18 +2258,18 @@ is_exit_without_lineno(basicblock *b) {
static int
duplicate_exits_without_lineno(cfg_builder *g)
{
assert(no_empty_basic_blocks(g));

int next_lbl = get_max_label(g->g_entryblock) + 1;

/* Copy all exit blocks without line number that are targets of a jump.
*/
basicblock *entryblock = g->g_entryblock;
for (basicblock *b = entryblock; b != NULL; b = b->b_next) {
cfg_instr *last = basicblock_last_instr(b);
assert(last != NULL);
if (last == NULL) {
continue;
}
if (is_jump(last)) {
basicblock *target = last->i_target;
basicblock *target = next_nonempty_block(last->i_target);
if (is_exit_without_lineno(target) && target->b_predecessors > 1) {
basicblock *new_target = copy_basicblock(g, target);
if (new_target == NULL) {
Expand Down Expand Up @@ -2367,9 +2326,10 @@ propagate_line_numbers(basicblock *entryblock) {
}
}
if (BB_HAS_FALLTHROUGH(b) && b->b_next->b_predecessors == 1) {
assert(b->b_next->b_iused);
if (b->b_next->b_instr[0].i_loc.lineno < 0) {
b->b_next->b_instr[0].i_loc = prev_location;
if (b->b_next->b_iused > 0) {
if (b->b_next->b_instr[0].i_loc.lineno < 0) {
b->b_next->b_instr[0].i_loc = prev_location;
}
}
}
if (is_jump(last)) {
Expand Down

0 comments on commit ea8bd20

Please sign in to comment.