From ef68aaa5e7b27cc05bb434f1a8a304ca52b562f5 Mon Sep 17 00:00:00 2001 From: Yan Yanchii Date: Fri, 14 Feb 2025 12:15:10 +0100 Subject: [PATCH 1/4] set location for noped out instructions after optimizations --- Lib/test/test_peepholer.py | 77 ++++++++++++++++++++++++++++++++ Python/flowgraph.c | 90 ++++++++++++++------------------------ 2 files changed, 110 insertions(+), 57 deletions(-) diff --git a/Lib/test/test_peepholer.py b/Lib/test/test_peepholer.py index 1c9db51972d575..f3676965812547 100644 --- a/Lib/test/test_peepholer.py +++ b/Lib/test/test_peepholer.py @@ -535,6 +535,83 @@ def test_folding_subscript(self): self.assertInBytecode(code, 'BINARY_OP', argval=subscr_argval) self.check_lnotab(code) + def test_constant_folding_reset_nop_location(self): + sources = [ + """ + (- + - + - + 1) + """, + + """ + (1 + + + 2 + + + 3) + """, + + """ + (1, + 2, + 3)[0] + """, + + """ + [1, + 2, + 3] + """, + + """ + {1, + 2, + 3} + """, + + """ + 1 in [ + 1, + 2, + 3 + ] + """, + + """ + 1 in { + 1, + 2, + 3 + } + """, + + """ + for _ in [1, + 2, + 3]: + pass + """, + + """ + for _ in [1, + 2, + x]: + pass + """, + + """ + for _ in {1, + 2, + 3}: + pass + """ + ] + + for source in sources: + code = compile(textwrap.dedent(source), '', 'single') + self.assertNotInBytecode(code, 'NOP') + def test_in_literal_list(self): def containtest(): return x in [a, b] diff --git a/Python/flowgraph.c b/Python/flowgraph.c index f9a7c6fe210c83..6cffa46bab7c53 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -126,6 +126,12 @@ is_jump(cfg_instr *i) _instr__ptr_->i_oparg = 0; \ } while (0); +#define INSTR_SET_LOC(I, LOC) \ + do { \ + cfg_instr *_instr__ptr_ = (I); \ + _instr__ptr_->i_loc = (LOC); \ + } while (0); + /***** Blocks *****/ /* Returns the offset of the next instruction in the current block's @@ -1382,18 +1388,22 @@ get_constant_sequence(basicblock *bb, int start, int size, /* Walk basic block backwards starting from "start" and change "count" number of - non-NOP instructions to NOP's. + non-NOP instructions to NOP's and set their i_loc info to "location" if provided. */ static void -nop_out(basicblock *bb, int start, int count) +nop_out(basicblock *bb, int start, int count, _Py_SourceLocation *location) { assert(start < bb->b_iused); for (; count > 0; start--) { assert(start >= 0); - if (bb->b_instr[start].i_opcode == NOP) { + cfg_instr *instr = &bb->b_instr[start]; + if (instr->i_opcode == NOP) { continue; } - INSTR_SET_OP0(&bb->b_instr[start], NOP); + INSTR_SET_OP0(instr, NOP); + if (location != NULL) { + INSTR_SET_LOC(instr, *location); + } count--; } } @@ -1422,8 +1432,8 @@ fold_tuple_of_constants(basicblock *bb, int n, PyObject *consts, PyObject *const assert(PyTuple_CheckExact(newconst) && PyTuple_GET_SIZE(newconst) == seq_size); int index = add_const(newconst, consts, const_cache); RETURN_IF_ERROR(index); - nop_out(bb, n-1, seq_size); - INSTR_SET_OP1(&bb->b_instr[n], LOAD_CONST, index); + nop_out(bb, n-1, seq_size, &instr->i_loc); + INSTR_SET_OP1(instr, LOAD_CONST, index); return SUCCESS; } @@ -1472,7 +1482,7 @@ optimize_lists_and_sets(basicblock *bb, int i, int nextop, } int index = add_const(newconst, consts, const_cache); RETURN_IF_ERROR(index); - nop_out(bb, i-1, seq_size); + nop_out(bb, i-1, seq_size, &instr->i_loc); if (contains_or_iter) { INSTR_SET_OP1(instr, LOAD_CONST, index); } @@ -1486,33 +1496,6 @@ optimize_lists_and_sets(basicblock *bb, int i, int nextop, return SUCCESS; } -/* - Walk basic block backwards starting from "start" to collect instruction pair - that loads consts skipping NOP's in between. -*/ -static bool -find_load_const_pair(basicblock *bb, int start, cfg_instr **first, cfg_instr **second) -{ - cfg_instr *second_load_const = NULL; - while (start >= 0) { - cfg_instr *inst = &bb->b_instr[start--]; - if (inst->i_opcode == NOP) { - continue; - } - if (!loads_const(inst->i_opcode)) { - return false; - } - if (second_load_const == NULL) { - second_load_const = inst; - continue; - } - *first = inst; - *second = second_load_const; - return true; - } - return false; -} - /* Determine opcode & oparg for freshly folded constant. */ static int newop_from_folded(PyObject *newconst, PyObject *consts, @@ -1534,27 +1517,25 @@ newop_from_folded(PyObject *newconst, PyObject *consts, } static int -optimize_if_const_op(basicblock *bb, int n, PyObject *consts, PyObject *const_cache) +optimize_if_const_binop(basicblock *bb, int i, PyObject *consts, PyObject *const_cache) { - cfg_instr *subscr = &bb->b_instr[n]; - assert(subscr->i_opcode == BINARY_OP); - if (subscr->i_oparg != NB_SUBSCR) { + cfg_instr *binop = &bb->b_instr[i]; + assert(binop->i_opcode == BINARY_OP); + if (binop->i_oparg != NB_SUBSCR) { /* TODO: support other binary ops */ return SUCCESS; } - cfg_instr *arg, *idx; - if (!find_load_const_pair(bb, n-1, &arg, &idx)) { + PyObject *pair; + RETURN_IF_ERROR(get_constant_sequence(bb, i-1, 2, consts, &pair)); + if (pair == NULL) { return SUCCESS; } - PyObject *o = NULL, *key = NULL; - if ((o = get_const_value(arg->i_opcode, arg->i_oparg, consts)) == NULL - || (key = get_const_value(idx->i_opcode, idx->i_oparg, consts)) == NULL) - { - goto error; - } - PyObject *newconst = PyObject_GetItem(o, key); - Py_DECREF(o); - Py_DECREF(key); + assert(PyTuple_CheckExact(pair) && PyTuple_Size(pair) == 2); + PyObject *left = PyTuple_GET_ITEM(pair, 0); + PyObject *right = PyTuple_GET_ITEM(pair, 1); + assert(left != NULL && right != NULL); + PyObject *newconst = PyObject_GetItem(left, right); + Py_DECREF(pair); if (newconst == NULL) { if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) { return ERROR; @@ -1564,14 +1545,9 @@ optimize_if_const_op(basicblock *bb, int n, PyObject *consts, PyObject *const_ca } int newopcode, newoparg; RETURN_IF_ERROR(newop_from_folded(newconst, consts, const_cache, &newopcode, &newoparg)); - INSTR_SET_OP1(subscr, newopcode, newoparg); - INSTR_SET_OP0(arg, NOP); - INSTR_SET_OP0(idx, NOP); + nop_out(bb, i-1, 2, &binop->i_loc); + INSTR_SET_OP1(binop, newopcode, newoparg); return SUCCESS; -error: - Py_XDECREF(o); - Py_XDECREF(key); - return ERROR; } #define VISITED (-1) @@ -2072,7 +2048,7 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) } break; case BINARY_OP: - RETURN_IF_ERROR(optimize_if_const_op(bb, i, consts, const_cache)); + RETURN_IF_ERROR(optimize_if_const_binop(bb, i, consts, const_cache)); break; } } From 0f95636ca4bb33c5102c885621d8514a9df27c94 Mon Sep 17 00:00:00 2001 From: Yan Yanchii Date: Fri, 14 Feb 2025 12:35:05 +0100 Subject: [PATCH 2/4] use NO_LOCATION --- Python/flowgraph.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Python/flowgraph.c b/Python/flowgraph.c index 6cffa46bab7c53..4aeaac8088cc00 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -1388,10 +1388,10 @@ get_constant_sequence(basicblock *bb, int start, int size, /* Walk basic block backwards starting from "start" and change "count" number of - non-NOP instructions to NOP's and set their i_loc info to "location" if provided. + non-NOP instructions to NOP's and set their location to NO_LOCATION. */ static void -nop_out(basicblock *bb, int start, int count, _Py_SourceLocation *location) +nop_out(basicblock *bb, int start, int count) { assert(start < bb->b_iused); for (; count > 0; start--) { @@ -1401,9 +1401,7 @@ nop_out(basicblock *bb, int start, int count, _Py_SourceLocation *location) continue; } INSTR_SET_OP0(instr, NOP); - if (location != NULL) { - INSTR_SET_LOC(instr, *location); - } + INSTR_SET_LOC(instr, NO_LOCATION); count--; } } @@ -1432,7 +1430,7 @@ fold_tuple_of_constants(basicblock *bb, int n, PyObject *consts, PyObject *const assert(PyTuple_CheckExact(newconst) && PyTuple_GET_SIZE(newconst) == seq_size); int index = add_const(newconst, consts, const_cache); RETURN_IF_ERROR(index); - nop_out(bb, n-1, seq_size, &instr->i_loc); + nop_out(bb, n-1, seq_size); INSTR_SET_OP1(instr, LOAD_CONST, index); return SUCCESS; } @@ -1482,7 +1480,7 @@ optimize_lists_and_sets(basicblock *bb, int i, int nextop, } int index = add_const(newconst, consts, const_cache); RETURN_IF_ERROR(index); - nop_out(bb, i-1, seq_size, &instr->i_loc); + nop_out(bb, i-1, seq_size); if (contains_or_iter) { INSTR_SET_OP1(instr, LOAD_CONST, index); } @@ -1545,7 +1543,7 @@ optimize_if_const_binop(basicblock *bb, int i, PyObject *consts, PyObject *const } int newopcode, newoparg; RETURN_IF_ERROR(newop_from_folded(newconst, consts, const_cache, &newopcode, &newoparg)); - nop_out(bb, i-1, 2, &binop->i_loc); + nop_out(bb, i-1, 2); INSTR_SET_OP1(binop, newopcode, newoparg); return SUCCESS; } From bdae73b4a3e73a85b6eb4e7351077aa2f3f646a9 Mon Sep 17 00:00:00 2001 From: Yan Yanchii Date: Fri, 14 Feb 2025 12:36:21 +0100 Subject: [PATCH 3/4] update tests --- Lib/test/test_peepholer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_peepholer.py b/Lib/test/test_peepholer.py index f3676965812547..4471c5129b96df 100644 --- a/Lib/test/test_peepholer.py +++ b/Lib/test/test_peepholer.py @@ -535,7 +535,7 @@ def test_folding_subscript(self): self.assertInBytecode(code, 'BINARY_OP', argval=subscr_argval) self.check_lnotab(code) - def test_constant_folding_reset_nop_location(self): + def test_constant_folding_remove_nop_location(self): sources = [ """ (- From cdf427a10e9ee4c1c4fdcc4fdfb0a7622781bba5 Mon Sep 17 00:00:00 2001 From: Yan Yanchii Date: Fri, 14 Feb 2025 14:06:49 +0100 Subject: [PATCH 4/4] set location for BUILD_SET/BUILD_LIST --- Python/flowgraph.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Python/flowgraph.c b/Python/flowgraph.c index 4aeaac8088cc00..38fb40831f3735 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -1487,6 +1487,9 @@ optimize_lists_and_sets(basicblock *bb, int i, int nextop, else { assert(i >= 2); assert(instr->i_opcode == BUILD_LIST || instr->i_opcode == BUILD_SET); + + INSTR_SET_LOC(&bb->b_instr[i-2], instr->i_loc); + INSTR_SET_OP1(&bb->b_instr[i-2], instr->i_opcode, 0); INSTR_SET_OP1(&bb->b_instr[i-1], LOAD_CONST, index); INSTR_SET_OP1(&bb->b_instr[i], instr->i_opcode == BUILD_LIST ? LIST_EXTEND : SET_UPDATE, 1);