gh-148817: Fold long lists/sets of constant elements into constant tuples/frozensets - #149016
Conversation
|
thanks for the PR @Eclips4 ! |
| LIST_APPEND 1 | ||
| CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE | ||
| LIST_APPEND/SET_ADD 1 | ||
| [CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE] <-- when intrinsic_at_i is true |
There was a problem hiding this comment.
Can intrinsic_at_i be renamed to something clearer? Maybe just expected_append?
| if (oparg == INTRINSIC_LIST_TO_TUPLE) { | ||
| if (nextop == GET_ITER) { | ||
| RETURN_IF_ERROR(fold_constant_intrinsic_list_to_tuple(bb, i, consts, const_cache, consts_index)); | ||
| /* If folding didn't apply, the list-to-tuple conversion |
There was a problem hiding this comment.
Is this related to the current PR? It seems like a separate optimisation.
Also, why is the to-tuple redundant only when the folding did not apply? If it's folded it's already a tuple, right?
There was a problem hiding this comment.
Yeah, you're right, if it's folded it's already a tuple, was I meant to say if it was folded, the intrinsic is no longer exists in this case. And yes, this is a separate optimization, so we can apply it as a follow up; I'll remove it.
Documentation build overview
51 files changed ·
|
|
This PR is stale because it has been open for 90 days with no activity. |
|
@Eclips4 @iritkatriel anything left to do here? |
|
I think this is ready, but it would be great if @iritkatriel could take another look. |
| 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. |
There was a problem hiding this comment.
Could we, instead of adding the expected_append arg, derive it from the current opcocde? (if i is a LIST_TO_TUPLE then it's true)?
| LIST_APPEND 1 | ||
| CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE | ||
| LIST_APPEND/SET_ADD 1 | ||
| [CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE] <-- optional |
There was a problem hiding this comment.
If there is no LIST_TO_TUPLE in the byte code, we allow ourselves to convert to tuple because we know that's ok in the context of in, etc. But if there is a LIST_TO_TUPLE, then we could apply the optimisation regardless of the context. Is that not worth doing?
| break; | ||
| case LIST_APPEND: | ||
| case SET_ADD: | ||
| if (oparg == 1 && (nextop == GET_ITER || nextop == CONTAINS_OP)) { |
There was a problem hiding this comment.
I think there may be other cases where the optimisation can be applied:
LIST_EXTEND, SET_UPDATE , UNPACK_SEQUENCE.
There was a problem hiding this comment.
I'll do that in follow-up, but from a quick look it seems to be true
|
Thanks @Eclips4 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15. |
|
Sorry, @Eclips4, I could not cleanly backport this to |
|
GH-155897 is a backport of this pull request to the 3.15 branch. |
|
GH-155898 is a backport of this pull request to the 3.14 branch. |
…tant tu… (#155898) gh-148817: Fold long lists/sets of constant elements into constant tuples/frozensets (#149016) Fold long lists/sets of constant elements into constant tuples/frozensets. (cherry picked from commit 084230e) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
in/not inandforloops #148817