Repository navigation
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 3 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 |
|---|---|---|
|
|
@@ -1507,34 +1507,46 @@ 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] <-- when expected_append is true | ||
| with: | ||
| LOAD_CONST (c1, c2, ... cN) | ||
| When expected_append is true, the instruction at `i` is 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. When expected_append is false, the instruction at `i` is the | ||
| trailing LIST_APPEND or SET_ADD itself, 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, | ||
| bool expected_append, | ||
| 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]; | ||
| 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; | ||
|
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. It might be more readable if we had |
||
|
|
||
| for (int pos = i - 1; pos >= 0; pos--) { | ||
| cfg_instr *instr = &bb->b_instr[pos]; | ||
|
|
@@ -1545,7 +1557,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; | ||
|
|
@@ -1557,7 +1569,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; | ||
|
|
@@ -1574,11 +1587,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.
Resolved
|
||
| return SUCCESS; | ||
| } | ||
| } | ||
|
|
@@ -1596,6 +1618,17 @@ fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | |
| return SUCCESS; | ||
| } | ||
|
|
||
| static int | ||
| fold_constant_intrinsic_list_to_tuple(basicblock *bb, int i, | ||
| PyObject *consts, PyObject *const_cache, | ||
| _Py_hashtable_t *consts_index) | ||
| { | ||
| assert(bb->b_instr[i].i_opcode == CALL_INTRINSIC_1); | ||
| assert(bb->b_instr[i].i_oparg == INTRINSIC_LIST_TO_TUPLE); | ||
| return fold_constant_seq_into_load_const(bb, i, true, | ||
| consts, const_cache, consts_index); | ||
| } | ||
|
|
||
| #define MIN_CONST_SEQUENCE_SIZE 3 | ||
| /* | ||
| Optimize lists and sets for: | ||
|
|
@@ -2517,6 +2550,14 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts, | |
| 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, false, | ||
| 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.
Could we, instead of adding the
expected_appendarg, derive it from the current opcocde? (ifiis aLIST_TO_TUPLEthen it's true)?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.
Yeah, good idea.