From fff4211465c5ad98b64ccd6a9ffa6479312b0e9d Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Mon, 9 Oct 2023 14:54:00 +0200 Subject: [PATCH] Make _LOAD_ATTR_PROPERTY viable; at the cost of hacks --- Include/internal/pycore_opcode_metadata.h | 1 + Python/abstract_interp_cases.c.h | 7 ++++ Python/bytecodes.c | 20 +++++------ Python/executor_cases.c.h | 20 +++++++++++ Python/generated_cases.c.h | 42 ++++++++++++++++++++--- Tools/cases_generator/stacking.py | 3 ++ 6 files changed, 78 insertions(+), 15 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 6176277b074ef69..b31ee0ef0f4c189 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1713,6 +1713,7 @@ const struct opcode_macro_expansion _PyOpcode_macro_expansion[OPCODE_MACRO_EXPAN [LOAD_ATTR_WITH_HINT] = { .nuops = 3, .uops = { { _GUARD_TYPE_VERSION, 2, 1 }, { _CHECK_ATTR_WITH_HINT, 0, 0 }, { _LOAD_ATTR_WITH_HINT, 1, 3 } } }, [LOAD_ATTR_SLOT] = { .nuops = 2, .uops = { { _GUARD_TYPE_VERSION, 2, 1 }, { _LOAD_ATTR_SLOT, 1, 3 } } }, [LOAD_ATTR_CLASS] = { .nuops = 2, .uops = { { _CHECK_ATTR_CLASS, 2, 1 }, { _LOAD_ATTR_CLASS, 4, 5 } } }, + [LOAD_ATTR_PROPERTY] = { .nuops = 8, .uops = { { _CHECK_PEP_523, 0, 0 }, { _GUARD_TYPE_VERSION, 2, 1 }, { _HELPER_LOAD_FUNC_FROM_CACHE, 4, 3 }, { _CHECK_FUNC_VERSION, 2, 7 }, { _LOAD_ATTR_PROPERTY, 0, 0 }, { _SET_IP, 7, 9 }, { _SAVE_CURRENT_IP, 0, 0 }, { _PUSH_FRAME, 0, 0 } } }, [STORE_ATTR_INSTANCE_VALUE] = { .nuops = 3, .uops = { { _GUARD_TYPE_VERSION, 2, 1 }, { _GUARD_DORV_VALUES, 0, 0 }, { _STORE_ATTR_INSTANCE_VALUE, 1, 3 } } }, [STORE_ATTR_SLOT] = { .nuops = 2, .uops = { { _GUARD_TYPE_VERSION, 2, 1 }, { _STORE_ATTR_SLOT, 1, 3 } } }, [COMPARE_OP] = { .nuops = 1, .uops = { { COMPARE_OP, 0, 0 } } }, diff --git a/Python/abstract_interp_cases.c.h b/Python/abstract_interp_cases.c.h index d3c0c5a3badef80..3bb6e53e267c4af 100644 --- a/Python/abstract_interp_cases.c.h +++ b/Python/abstract_interp_cases.c.h @@ -524,6 +524,13 @@ break; } + case _LOAD_ATTR_PROPERTY: { + STACK_SHRINK(1); + PARTITIONNODE_OVERWRITE((_Py_PARTITIONNODE_t *)PARTITIONNODE_NULLROOT, PEEK(-(-1)), true); + PARTITIONNODE_OVERWRITE((_Py_PARTITIONNODE_t *)PARTITIONNODE_NULLROOT, PEEK(-(0)), true); + break; + } + case _GUARD_DORV_VALUES: { break; } diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 83ab424fdd77f2b..86a1e7c72aaf6a0 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1992,29 +1992,26 @@ dummy_func( unused/2 + _LOAD_ATTR_CLASS; - op(_HELPER_LOAD_FUNC_FROM_CACHE, (fget/4 -- func: PyFunctionObject *)) { + op(_HELPER_LOAD_FUNC_FROM_CACHE, (fget/4 -- func: PyFunctionObject*)) { assert(Py_IS_TYPE(fget, &PyFunction_Type)); func = (PyFunctionObject *)fget; } - op(_CHECK_FUNC_VERSION, (func_version/2, func: PyFunctionObject * -- func: PyFunctionObject *)) { + op(_CHECK_FUNC_VERSION, (func_version/2, func: PyFunctionObject* -- func: PyFunctionObject*)) { assert(func_version != 0); DEOPT_IF(func->func_version != func_version); } - op(_LOAD_ATTR_PROPERTY, (owner, func: PyFunctionObject * -- unused, unused if (0))) { + op(_LOAD_ATTR_PROPERTY, (owner, func: PyFunctionObject* -- new_frame: _PyInterpreterFrame*, unused if (0))) { assert((oparg & 1) == 0); PyCodeObject *code = (PyCodeObject *)func->func_code; assert(code->co_argcount == 1); DEOPT_IF(!_PyThreadState_HasStackSpace(tstate, code->co_framesize)); STAT_INC(LOAD_ATTR, hit); - _PyInterpreterFrame *new_frame = _PyFrame_PushUnchecked(tstate, Py_NewRef(func), 1); - // Manipulate stack directly because we exit with DISPATCH_INLINED(). - STACK_SHRINK(1); + Py_INCREF(func); + new_frame = _PyFrame_PushUnchecked(tstate, func, 1); new_frame->localsplus[0] = owner; - SKIP_OVER(INLINE_CACHE_ENTRIES_LOAD_ATTR); - frame->return_offset = 0; - DISPATCH_INLINED(new_frame); + stack_pointer[-1] = (PyObject *)new_frame; // Unfortunately this is needed } macro(LOAD_ATTR_PROPERTY) = @@ -2023,7 +2020,10 @@ dummy_func( _GUARD_TYPE_VERSION + _HELPER_LOAD_FUNC_FROM_CACHE + _CHECK_FUNC_VERSION + - _LOAD_ATTR_PROPERTY; + _LOAD_ATTR_PROPERTY + + _SET_IP + // Tier 2 only; special-cased oparg + _SAVE_CURRENT_IP + // Sets frame->prev_instr + _PUSH_FRAME; inst(LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN, (unused/1, type_version/2, func_version/2, getattribute/4, owner -- unused, unused if (0))) { assert((oparg & 1) == 0); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 651e59a6ba21be9..ed2f84f2f12eba0 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1853,6 +1853,26 @@ break; } + case _LOAD_ATTR_PROPERTY: { + PyFunctionObject *func; + PyObject *owner; + _PyInterpreterFrame *new_frame; + func = (PyFunctionObject *)stack_pointer[-1]; + owner = stack_pointer[-2]; + assert((oparg & 1) == 0); + PyCodeObject *code = (PyCodeObject *)func->func_code; + assert(code->co_argcount == 1); + DEOPT_IF(!_PyThreadState_HasStackSpace(tstate, code->co_framesize), _LOAD_ATTR_PROPERTY); + STAT_INC(LOAD_ATTR, hit); + Py_INCREF(func); + new_frame = _PyFrame_PushUnchecked(tstate, func, 1); + new_frame->localsplus[0] = owner; + stack_pointer[-1] = (PyObject *)new_frame; // Unfortunately this is needed + STACK_SHRINK(1); + stack_pointer[-1] = (PyObject *)new_frame; + break; + } + case _GUARD_DORV_VALUES: { PyObject *owner; owner = stack_pointer[-1]; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 725a398d7c4f9c9..dd84c3c5a8af680 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2578,6 +2578,7 @@ TARGET(LOAD_ATTR_PROPERTY) { PyObject *owner; PyFunctionObject *func; + _PyInterpreterFrame *new_frame; // _CHECK_PEP_523 { DEOPT_IF(tstate->interp->eval_frame, LOAD_ATTR); @@ -2609,14 +2610,45 @@ assert(code->co_argcount == 1); DEOPT_IF(!_PyThreadState_HasStackSpace(tstate, code->co_framesize), LOAD_ATTR); STAT_INC(LOAD_ATTR, hit); - _PyInterpreterFrame *new_frame = _PyFrame_PushUnchecked(tstate, Py_NewRef(func), 1); - // Manipulate stack directly because we exit with DISPATCH_INLINED(). - STACK_SHRINK(1); + Py_INCREF(func); + new_frame = _PyFrame_PushUnchecked(tstate, func, 1); new_frame->localsplus[0] = owner; - SKIP_OVER(INLINE_CACHE_ENTRIES_LOAD_ATTR); + stack_pointer[-1] = (PyObject *)new_frame; // Unfortunately this is needed + } + // _SAVE_CURRENT_IP + next_instr += 9; + { + #if TIER_ONE + frame->prev_instr = next_instr - 1; + #endif + #if TIER_TWO + // Relies on a preceding _SET_IP + frame->prev_instr--; + #endif + } + // _PUSH_FRAME + new_frame = (_PyInterpreterFrame *)stack_pointer[-1]; + STACK_SHRINK(1); + { + // Write it out explicitly because it's subtly different. + // Eventually this should be the only occurrence of this code. frame->return_offset = 0; - DISPATCH_INLINED(new_frame); + assert(tstate->interp->eval_frame == NULL); + STORE_SP(); + new_frame->previous = frame; + CALL_STAT_INC(inlined_py_calls); + frame = tstate->current_frame = new_frame; + tstate->py_recursion_remaining--; + LOAD_SP(); + LOAD_IP(); + #if LLTRACE && TIER_ONE + lltrace = maybe_lltrace_resume_frame(frame, &entry_frame, GLOBALS()); + if (lltrace < 0) { + goto exit_unwind; + } + #endif } + DISPATCH(); } TARGET(LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN) { diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index bba2db8b059da86..c221415968d9838 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -137,6 +137,8 @@ def as_variable(self, lax: bool = False) -> str: if not lax: # Check that we're not reading or writing above stack top. # Skip this for output variable initialization (lax=True). + if not (self.effect in self.offset.deep and not self.offset.high): # DO NOT COMMIT + return res # DO NOT COMMIT assert ( self.effect in self.offset.deep and not self.offset.high ), f"Push or pop above current stack level: {res}" @@ -473,6 +475,7 @@ def write_components( def assert_no_pokes(managers: list[EffectManager]) -> None: + return # DO NOT COMMIT for mgr in managers: for poke in mgr.pokes: if not poke.effect.size and poke.effect.name not in mgr.instr.unmoved_names: