From 2deceef1810840338284fb4b2840368c1e4fa496 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 | 18 +++++------ Python/executor_cases.c.h | 20 ++++++++++++ Python/generated_cases.c.h | 38 ++++++++++++++++++++--- Tools/cases_generator/stacking.py | 3 ++ 6 files changed, 74 insertions(+), 13 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 94b44cb667b9872..20d98cd033b3815 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1865,6 +1865,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 = 7, .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 }, { _SAVE_RETURN_OFFSET, 7, 9 }, { _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_FLOAT] = { .nuops = 1, .uops = { { COMPARE_OP_FLOAT, 0, 0 } } }, diff --git a/Python/abstract_interp_cases.c.h b/Python/abstract_interp_cases.c.h index 20ed4d5d2e32eef..ee3b90a57d01d76 100644 --- a/Python/abstract_interp_cases.c.h +++ b/Python/abstract_interp_cases.c.h @@ -532,6 +532,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 992081e3ba98b79..e39fbc8a7808f09 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2043,28 +2043,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; - frame->return_offset = (uint16_t)(next_instr - this_instr); - DISPATCH_INLINED(new_frame); + stack_pointer[-1] = (PyObject *)new_frame; // Unfortunately this is needed } macro(LOAD_ATTR_PROPERTY) = @@ -2073,7 +2071,9 @@ dummy_func( _GUARD_TYPE_VERSION + _HELPER_LOAD_FUNC_FROM_CACHE + _CHECK_FUNC_VERSION + - _LOAD_ATTR_PROPERTY; + _LOAD_ATTR_PROPERTY + + _SAVE_RETURN_OFFSET + + _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 5098d7dca78df88..6a98d3700613131 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1811,6 +1811,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 a840e110cc3679d..53368543c255d02 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2954,6 +2954,7 @@ INSTRUCTION_STATS(LOAD_ATTR_PROPERTY); PyObject *owner; PyFunctionObject *func; + _PyInterpreterFrame *new_frame; // _CHECK_PEP_523 { DEOPT_IF(tstate->interp->eval_frame, LOAD_ATTR); @@ -2985,13 +2986,42 @@ 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; + stack_pointer[-1] = (PyObject *)new_frame; // Unfortunately this is needed + } + // _SAVE_RETURN_OFFSET + { + #if TIER_ONE frame->return_offset = (uint16_t)(next_instr - this_instr); - DISPATCH_INLINED(new_frame); + #endif + #if TIER_TWO + frame->return_offset = oparg; + #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. + 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(0); + #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 123e38c524f49d3..1b56e3855989429 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}" @@ -478,6 +480,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: