-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-148817: Fold long lists/sets of constant elements into constant tuples/frozensets #149016
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
Changes from all commits
ddd3875
3510ec1
0c2f097
79d68e0
737095e
fa30806
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Fold large constant list and set literals used as the iterable of a | ||
| :keyword:`for` loop or ``in``/``not in`` test into a constant | ||
| :class:`tuple` or :class:`frozenset`, restoring an optimization | ||
| previously done by the AST optimizer that was lost when constant | ||
| folding moved to the CFG. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1569,34 +1569,48 @@ fold_tuple_of_constants(basicblock *bb, int i, PyObject *consts, | |
| } | ||
|
|
||
| /* Replace: | ||
| BUILD_LIST 0 | ||
| BUILD_LIST/BUILD_SET 0 | ||
| LOAD_CONST c1 | ||
| LIST_APPEND 1 | ||
| LIST_APPEND/SET_ADD 1 | ||
| LOAD_CONST c2 | ||
| LIST_APPEND 1 | ||
| LIST_APPEND/SET_ADD 1 | ||
| ... | ||
| LOAD_CONST cN | ||
| LIST_APPEND 1 | ||
| CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE | ||
| LIST_APPEND/SET_ADD 1 | ||
| [CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE] <-- optional | ||
| with: | ||
| LOAD_CONST (c1, c2, ... cN) | ||
| The instruction at `i` is either the LIST_TO_TUPLE intrinsic (so the | ||
| immediately preceding non-NOP instruction is expected to be a | ||
| LIST_APPEND, and only the BUILD_LIST/LIST_APPEND form is considered), | ||
| or the trailing LIST_APPEND or SET_ADD itself, in which case the | ||
| matching BUILD_LIST/BUILD_SET start is selected from its opcode, and | ||
| for sets the result is wrapped in a frozenset. | ||
| */ | ||
| static int | ||
| fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | ||
| PyObject *consts, PyObject *const_cache, | ||
| _Py_hashtable_t *consts_index) | ||
| fold_constant_seq_into_load_const(basicblock *bb, int i, | ||
| PyObject *consts, PyObject *const_cache, | ||
| _Py_hashtable_t *consts_index) | ||
| { | ||
| assert(PyDict_CheckExact(const_cache)); | ||
| assert(PyList_CheckExact(consts)); | ||
| assert(i >= 0); | ||
| assert(i < bb->b_iused); | ||
|
|
||
| cfg_instr *intrinsic = &bb->b_instr[i]; | ||
| assert(intrinsic->i_opcode == CALL_INTRINSIC_1); | ||
| assert(intrinsic->i_oparg == INTRINSIC_LIST_TO_TUPLE); | ||
|
|
||
| cfg_instr *target = &bb->b_instr[i]; | ||
| assert(target->i_opcode == LIST_APPEND || target->i_opcode == SET_ADD || | ||
| (target->i_opcode == CALL_INTRINSIC_1 && | ||
| target->i_oparg == INTRINSIC_LIST_TO_TUPLE)); | ||
| bool expected_append = target->i_opcode == CALL_INTRINSIC_1; | ||
| int append_op = expected_append ? LIST_APPEND : target->i_opcode; | ||
| assert(append_op == LIST_APPEND || append_op == SET_ADD); | ||
| int build_op = append_op == LIST_APPEND ? BUILD_LIST : BUILD_SET; | ||
| int consts_found = 0; | ||
| bool expect_append = true; | ||
| /* Walking backward from `i`, we expect LIST_APPEND/SET_ADD and | ||
| LOAD_CONST to alternate. If `i` is the trailing LIST_TO_TUPLE | ||
| intrinsic, the next instruction back is an APPEND. If `i` is the | ||
| trailing APPEND itself, the next instruction back is a LOAD_CONST. */ | ||
| bool expect_append = expected_append; | ||
|
|
||
| for (int pos = i - 1; pos >= 0; pos--) { | ||
| cfg_instr *instr = &bb->b_instr[pos]; | ||
|
|
@@ -1607,7 +1621,7 @@ fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | |
| continue; | ||
| } | ||
|
|
||
| if (opcode == BUILD_LIST && oparg == 0) { | ||
| if (opcode == build_op && oparg == 0) { | ||
| if (!expect_append) { | ||
| /* Not a sequence start. */ | ||
| return SUCCESS; | ||
|
|
@@ -1619,7 +1633,8 @@ fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | |
| return ERROR; | ||
| } | ||
|
|
||
| for (int newpos = i - 1; newpos >= pos; newpos--) { | ||
| int newpos_start = expected_append ? i - 1 : i; | ||
| for (int newpos = newpos_start; newpos >= pos; newpos--) { | ||
| instr = &bb->b_instr[newpos]; | ||
| if (instr->i_opcode == NOP) { | ||
| continue; | ||
|
|
@@ -1636,11 +1651,20 @@ fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | |
| nop_out(&instr, 1); | ||
| } | ||
| assert(consts_found == 0); | ||
| return instr_make_load_const(intrinsic, newconst, consts, const_cache, consts_index); | ||
|
|
||
| if (build_op == BUILD_SET) { | ||
| PyObject *frozen = PyFrozenSet_New(newconst); | ||
| Py_DECREF(newconst); | ||
| if (frozen == NULL) { | ||
| return ERROR; | ||
| } | ||
| newconst = frozen; | ||
| } | ||
| return instr_make_load_const(target, newconst, consts, const_cache, consts_index); | ||
| } | ||
|
|
||
| if (expect_append) { | ||
| if (opcode != LIST_APPEND || oparg != 1) { | ||
| if (opcode != append_op || oparg != 1) { | ||
|
iritkatriel marked this conversation as resolved.
|
||
| return SUCCESS; | ||
| } | ||
| } | ||
|
|
@@ -2579,17 +2603,22 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts, | |
| break; | ||
| case CALL_INTRINSIC_1: | ||
| if (oparg == INTRINSIC_LIST_TO_TUPLE) { | ||
| if (nextop == GET_ITER) { | ||
| RETURN_IF_ERROR(fold_constant_seq_into_load_const(bb, i, consts, const_cache, consts_index)); | ||
| if (inst->i_opcode == CALL_INTRINSIC_1 && nextop == GET_ITER) { | ||
| INSTR_SET_OP0(inst, NOP); | ||
| } | ||
| else { | ||
| RETURN_IF_ERROR(fold_constant_intrinsic_list_to_tuple(bb, i, consts, const_cache, consts_index)); | ||
| } | ||
| } | ||
| else if (oparg == INTRINSIC_UNARY_POSITIVE) { | ||
| RETURN_IF_ERROR(fold_const_unaryop(bb, i, consts, const_cache, consts_index)); | ||
| } | ||
| break; | ||
| case LIST_APPEND: | ||
| case SET_ADD: | ||
| if (oparg == 1 && (nextop == GET_ITER || nextop == CONTAINS_OP)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there may be other cases where the optimisation can be applied:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do that in follow-up, but from a quick look it seems to be true |
||
| RETURN_IF_ERROR(fold_constant_seq_into_load_const( | ||
| bb, i, consts, const_cache, consts_index)); | ||
| } | ||
| break; | ||
| case BINARY_OP: | ||
| RETURN_IF_ERROR(fold_const_binop(bb, i, consts, const_cache, consts_index)); | ||
| break; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no
LIST_TO_TUPLEin the byte code, we allow ourselves to convert to tuple because we know that's ok in the context ofin, etc. But if there is aLIST_TO_TUPLE, then we could apply the optimisation regardless of the context. Is that not worth doing?