GH-128939: Refactor JIT optimize structs #128940
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1465,6 +1465,25 @@ def f(l: complex, r: complex) -> None: | |
| with self.subTest(l=l, r=r, x=x, y=y): | ||
| script_helper.assert_python_ok("-c", s) | ||
|
|
||
| def test_symbols_flow_through_tuples(self): | ||
| def testfunc(n): | ||
| for _ in range(n): | ||
| a = 1 | ||
| b = 2 | ||
| t = a, b | ||
| x, y = t | ||
| r = x + y | ||
| return r | ||
|
|
||
| res, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD) | ||
| self.assertEqual(res, 3) | ||
| self.assertIsNotNone(ex) | ||
| uops = get_opnames(ex) | ||
| self.assertIn("_BINARY_OP_ADD_INT", uops) | ||
| self.assertNotIn("_GUARD_BOTH_INT", uops) | ||
| self.assertNotIn("_GUARD_NOS_INT", uops) | ||
| self.assertNotIn("_GUARD_TOS_INT", uops) | ||
|
There was a problem hiding this comment. This kind of check can easily pass by accident if there's typo or bytecodes change. Would be good to have a helper that asserts also that the thing not in is an actual uop id. There was a problem hiding this comment. It would. |
||
|
|
||
| def test_decref_escapes(self): | ||
| class Convert9999ToNone: | ||
| def __del__(self): | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1842,16 +1842,16 @@ def test_overridden_abstract_args(self): | |
| """ | ||
| output = """ | ||
| case OP: { | ||
| JitOptSymbol *arg1; | ||
| JitOptSymbol *out; | ||
| arg1 = stack_pointer[-1]; | ||
| out = EGGS(arg1); | ||
| stack_pointer[-1] = out; | ||
| break; | ||
| } | ||
|
|
||
| case OP2: { | ||
| JitOptSymbol *out; | ||
| out = sym_new_not_null(ctx); | ||
| stack_pointer[-1] = out; | ||
| break; | ||
|
|
@@ -1876,14 +1876,14 @@ def test_no_overridden_case(self): | |
| """ | ||
| output = """ | ||
| case OP: { | ||
| JitOptSymbol *out; | ||
| out = sym_new_not_null(ctx); | ||
| stack_pointer[-1] = out; | ||
| break; | ||
| } | ||
|
|
||
| case OP2: { | ||
| JitOptSymbol *out; | ||
| out = NULL; | ||
| stack_pointer[-1] = out; | ||
| break; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ | |
| #include "pycore_interp.h" // _PyInterpreterState_GetConfigCopy() | ||
| #include "pycore_long.h" // _PyLong_Sign() | ||
| #include "pycore_object.h" // _PyObject_IsFreed() | ||
| #include "pycore_optimizer.h" // JitOptSymbol, etc. | ||
| #include "pycore_pathconfig.h" // _PyPathConfig_ClearGlobal() | ||
| #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() | ||
| #include "pycore_pylifecycle.h" // _PyInterpreterConfig_AsDict() | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -368,13 +368,17 @@ remove_globals(_PyInterpreterFrame *frame, _PyUOpInstruction *buffer, | |
| #define sym_truthiness _Py_uop_sym_truthiness | ||
| #define frame_new _Py_uop_frame_new | ||
| #define frame_pop _Py_uop_frame_pop | ||
| #define sym_new_tuple _Py_uop_sym_new_tuple | ||
| #define sym_tuple_getitem _Py_uop_sym_tuple_getitem | ||
| #define sym_tuple_length _Py_uop_sym_tuple_length | ||
| #define sym_is_immortal _Py_uop_sym_is_immortal | ||
|
|
||
| static int | ||
| optimize_to_bool( | ||
| _PyUOpInstruction *this_instr, | ||
| JitOptContext *ctx, | ||
| JitOptSymbol *value, | ||
| JitOptSymbol **result_ptr) | ||
| { | ||
| if (sym_matches_type(value, &PyBool_Type)) { | ||
| REPLACE_OP(this_instr, _NOP, 0, 0); | ||
|
|
@@ -460,8 +464,8 @@ optimize_uops( | |
| ) | ||
| { | ||
|
|
||
| JitOptContext context; | ||
| JitOptContext *ctx = &context; | ||
| uint32_t opcode = UINT16_MAX; | ||
| int curr_space = 0; | ||
| int max_space = 0; | ||
|
|
@@ -486,7 +490,7 @@ optimize_uops( | |
|
|
||
| int oparg = this_instr->oparg; | ||
| opcode = this_instr->opcode; | ||
| JitOptSymbol **stack_pointer = ctx->frame->stack_pointer; | ||
|
|
||
| #ifdef Py_DEBUG | ||
| if (get_lltrace() >= 3) { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,6 @@ | |
|
|
||
| #define op(name, ...) /* NAME is ignored */ | ||
|
|
||
| typedef struct _Py_UOpsAbstractFrame _Py_UOpsAbstractFrame; | ||
|
|
||
| /* Shortened forms for convenience */ | ||
|
|
@@ -32,13 +30,17 @@ typedef struct _Py_UOpsAbstractFrame _Py_UOpsAbstractFrame; | |
| #define sym_is_bottom _Py_uop_sym_is_bottom | ||
| #define frame_new _Py_uop_frame_new | ||
| #define frame_pop _Py_uop_frame_pop | ||
| #define sym_new_tuple _Py_uop_sym_new_tuple | ||
| #define sym_tuple_getitem _Py_uop_sym_tuple_getitem | ||
| #define sym_tuple_length _Py_uop_sym_tuple_length | ||
| #define sym_is_immortal _Py_uop_sym_is_immortal | ||
|
|
||
| extern int | ||
| optimize_to_bool( | ||
| _PyUOpInstruction *this_instr, | ||
| JitOptContext *ctx, | ||
| JitOptSymbol *value, | ||
| JitOptSymbol **result_ptr); | ||
|
|
||
| extern void | ||
| eliminate_pop_guard(_PyUOpInstruction *this_instr, bool exit); | ||
|
|
@@ -50,17 +52,17 @@ dummy_func(void) { | |
|
|
||
| PyCodeObject *co; | ||
| int oparg; | ||
| JitOptSymbol *flag; | ||
| JitOptSymbol *left; | ||
| JitOptSymbol *right; | ||
| JitOptSymbol *value; | ||
| JitOptSymbol *res; | ||
| JitOptSymbol *iter; | ||
| JitOptSymbol *top; | ||
| JitOptSymbol *bottom; | ||
| _Py_UOpsAbstractFrame *frame; | ||
| _Py_UOpsAbstractFrame *new_frame; | ||
| JitOptContext *ctx; | ||
| _PyUOpInstruction *this_instr; | ||
| _PyBloomFilter *dependencies; | ||
| int modified; | ||
|
|
@@ -85,7 +87,7 @@ dummy_func(void) { | |
|
|
||
| op(_LOAD_FAST_AND_CLEAR, (-- value)) { | ||
| value = GETLOCAL(oparg); | ||
| JitOptSymbol *temp = sym_new_null(ctx); | ||
| GETLOCAL(oparg) = temp; | ||
| } | ||
|
|
||
|
|
@@ -365,7 +367,7 @@ dummy_func(void) { | |
| } | ||
|
|
||
| op(_BINARY_OP_INPLACE_ADD_UNICODE, (left, right -- )) { | ||
| JitOptSymbol *res; | ||
| if (sym_is_const(left) && sym_is_const(right) && | ||
| sym_matches_type(left, &PyUnicode_Type) && sym_matches_type(right, &PyUnicode_Type)) { | ||
| PyObject *temp = PyUnicode_Concat(sym_get_const(left), sym_get_const(right)); | ||
|
|
@@ -949,6 +951,22 @@ dummy_func(void) { | |
| res = sym_new_const(ctx, Py_True); | ||
| } | ||
|
|
||
| op(_BUILD_TUPLE, (values[oparg] -- tup)) { | ||
| tup = sym_new_tuple(ctx, oparg, values); | ||
| } | ||
|
|
||
| op(_UNPACK_SEQUENCE_TWO_TUPLE, (seq -- val1, val0)) { | ||
| val0 = sym_tuple_getitem(ctx, seq, 0); | ||
| val1 = sym_tuple_getitem(ctx, seq, 1); | ||
| } | ||
|
|
||
| op(_UNPACK_SEQUENCE_TUPLE, (seq -- values[oparg])) { | ||
| for (int i = 0; i < oparg; i++) { | ||
| values[i] = sym_tuple_getitem(ctx, seq, i); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| // END BYTECODES // | ||
|
|
||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We could probably just get the version by doing
cls.type->tp_version_tag, right? Having both here just creates opportunities for them to be stale or out-of-sync, I think.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.
sym->cls.type->tp_version_tagis the version tag of the value's class now.sym->version.versionis the version tag it would have should execution reach this point in the code. Not quite the same thing.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.
I would imagine that a
sym->cls.versionthat's out-of-sync withsym->cls.type->tp_version_tagshould result in the bottom type, then, right? It's not possible in future code, since the cached tag is stale.