Skip to content
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

gh-126835: Move optimization of constant sequence creation from codegen to CFG #129426

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
4 changes: 2 additions & 2 deletions Lib/test/test_dis.py
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ def loop_test():
%3d RESUME_CHECK 0

%3d BUILD_LIST 0
LOAD_CONST_MORTAL 0 ((1, 2, 3))
LOAD_CONST_MORTAL 1 ((1, 2, 3))
LIST_EXTEND 1
LOAD_SMALL_INT 3
BINARY_OP 5 (*)
Expand All @@ -908,7 +908,7 @@ def loop_test():

%3d L2: END_FOR
POP_ITER
LOAD_CONST_IMMORTAL 1 (None)
LOAD_CONST_IMMORTAL 0 (None)
RETURN_VALUE
""" % (loop_test.__code__.co_firstlineno,
loop_test.__code__.co_firstlineno + 1,
Expand Down
43 changes: 0 additions & 43 deletions Python/codegen.c
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,6 @@ static int codegen_subscript(compiler *, expr_ty);
static int codegen_slice_two_parts(compiler *, expr_ty);
static int codegen_slice(compiler *, expr_ty);

static bool are_all_items_const(asdl_expr_seq *, Py_ssize_t, Py_ssize_t);


static int codegen_with(compiler *, stmt_ty, int);
static int codegen_async_with(compiler *, stmt_ty, int);
static int codegen_async_for(compiler *, stmt_ty);
Expand Down Expand Up @@ -3210,34 +3207,6 @@ starunpack_helper_impl(compiler *c, location loc,
int build, int add, int extend, int tuple)
{
Py_ssize_t n = asdl_seq_LEN(elts);
if (!injected_arg && n > 2 && are_all_items_const(elts, 0, n)) {
PyObject *folded = PyTuple_New(n);
if (folded == NULL) {
return ERROR;
}
for (Py_ssize_t i = 0; i < n; i++) {
PyObject *val = ((expr_ty)asdl_seq_GET(elts, i))->v.Constant.value;
PyTuple_SET_ITEM(folded, i, Py_NewRef(val));
}
if (tuple && !pushed) {
ADDOP_LOAD_CONST_NEW(c, loc, folded);
} else {
if (add == SET_ADD) {
Py_SETREF(folded, PyFrozenSet_New(folded));
if (folded == NULL) {
return ERROR;
}
}
ADDOP_I(c, loc, build, pushed);
ADDOP_LOAD_CONST_NEW(c, loc, folded);
ADDOP_I(c, loc, extend, 1);
if (tuple) {
ADDOP_I(c, loc, CALL_INTRINSIC_1, INTRINSIC_LIST_TO_TUPLE);
}
}
return SUCCESS;
}

int big = n + pushed + (injected_arg ? 1 : 0) > STACK_USE_GUIDELINE;
int seen_star = 0;
for (Py_ssize_t i = 0; i < n; i++) {
Expand Down Expand Up @@ -3389,18 +3358,6 @@ codegen_set(compiler *c, expr_ty e)
BUILD_SET, SET_ADD, SET_UPDATE, 0);
}

static bool
are_all_items_const(asdl_expr_seq *seq, Py_ssize_t begin, Py_ssize_t end)
{
for (Py_ssize_t i = begin; i < end; i++) {
expr_ty key = (expr_ty)asdl_seq_GET(seq, i);
if (key == NULL || key->kind != Constant_kind) {
return false;
}
}
return true;
}

static int
codegen_subdict(compiler *c, expr_ty e, Py_ssize_t begin, Py_ssize_t end)
{
Expand Down
86 changes: 82 additions & 4 deletions Python/flowgraph.c
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,17 @@ add_const(PyObject *newconst, PyObject *consts, PyObject *const_cache)
return (int)index;
}

static int
is_sequence_constant(cfg_instr *inst, int n)
{
for (int i = 0; i < n; i++) {
if(!loads_const(inst[i].i_opcode)) {
return 0;
}
}
return 1;
}

/* Replace LOAD_CONST c1, LOAD_CONST c2 ... LOAD_CONST cn, BUILD_TUPLE n
with LOAD_CONST (c1, c2, ... cn).
The consts table must still be in list form so that the
Expand All @@ -1353,10 +1364,8 @@ fold_tuple_on_constants(PyObject *const_cache,
assert(inst[n].i_opcode == BUILD_TUPLE);
assert(inst[n].i_oparg == n);

for (int i = 0; i < n; i++) {
if (!loads_const(inst[i].i_opcode)) {
return SUCCESS;
}
if (!is_sequence_constant(inst, n)) {
return SUCCESS;
}

/* Buildup new tuple of constants */
Expand Down Expand Up @@ -1384,6 +1393,61 @@ fold_tuple_on_constants(PyObject *const_cache,
return SUCCESS;
}

// Replace LOAD_CONST x, LOAD_CONST y, LOAD_CONST z, BUILD_LIST 3
// with BUILD_LIST 0, LOAD_CONST (x, y, z), LIST_EXTEND 1
iritkatriel marked this conversation as resolved.
Show resolved Hide resolved
// or BUILD_SET & SET_UPDATE respectively.
static int
optimize_const_sequence(PyObject *const_cache,
cfg_instr* inst,
int n, PyObject *consts,
int list, int build, int extend)
{
assert(PyDict_CheckExact(const_cache));
assert(PyList_CheckExact(consts));
assert(inst[n].i_oparg == n);
if (list) {
assert(inst[n].i_opcode == BUILD_LIST);
}
else {
assert(inst[n].i_opcode == BUILD_SET);
}
Eclips4 marked this conversation as resolved.
Show resolved Hide resolved
iritkatriel marked this conversation as resolved.
Show resolved Hide resolved

if (n < 3 || !is_sequence_constant(inst, n)) {
iritkatriel marked this conversation as resolved.
Show resolved Hide resolved
return SUCCESS;
}
PyObject *newconst = PyTuple_New(n);
if (newconst == NULL) {
return ERROR;
}
for (int i = 0; i < n; i++) {
int op = inst[i].i_opcode;
int arg = inst[i].i_oparg;
PyObject *constant = get_const_value(op, arg, consts);
if (constant == NULL) {
return ERROR;
}
PyTuple_SET_ITEM(newconst, i, constant);
}
if (!list) {
PyObject *frozenset = PyFrozenSet_New(newconst);
if (frozenset == NULL) {
return ERROR;
}
Py_SETREF(newconst, frozenset);
}
int index = add_const(newconst, consts, const_cache);
if (index < 0) {
return ERROR;
}
Eclips4 marked this conversation as resolved.
Show resolved Hide resolved
INSTR_SET_OP1(&inst[0], build, 0);
for (int i = 1; i < n - 1; i++) {
INSTR_SET_OP0(&inst[i], NOP);
}
INSTR_SET_OP1(&inst[n-1], LOAD_CONST, index);
INSTR_SET_OP1(&inst[n], extend, 1);
return SUCCESS;
}

#define VISITED (-1)

// Replace an arbitrary run of SWAPs and NOPs with an optimal one that has the
Expand Down Expand Up @@ -1751,6 +1815,20 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts)
}
}
break;
case BUILD_LIST:
if (i >= oparg) {
if (optimize_const_sequence(const_cache, inst-oparg, oparg, consts, 1, BUILD_LIST, LIST_EXTEND)) {
Eclips4 marked this conversation as resolved.
Show resolved Hide resolved
goto error;
}
}
break;
case BUILD_SET:
if (i >= oparg) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it not a compiler bug if i < oparg?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's not. But if you're asking about removing this condition, I tried it and there was something completely wrong. The address sanitizer reported a heap-buffer-overflow error in that case. You can see this in this PR, before the ea07a50

if (optimize_const_sequence(const_cache, inst-oparg, oparg, consts, 0, BUILD_SET, SET_UPDATE)) {
Eclips4 marked this conversation as resolved.
Show resolved Hide resolved
goto error;
}
}
break;
case POP_JUMP_IF_NOT_NONE:
case POP_JUMP_IF_NONE:
switch (target->i_opcode) {
Expand Down
Loading