GH-109214: _SET_IP before _PUSH_FRAME (but not _POP_FRAME) (GH-111001)

This commit is contained in:
Brandt Bucher 2023-10-24 13:27:42 -07:00 committed by GitHub
parent c0ea67dd0d
commit e5168ff3f8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 28 additions and 44 deletions

View file

@ -1644,8 +1644,8 @@ const struct opcode_macro_expansion _PyOpcode_macro_expansion[OPCODE_MACRO_EXPAN
[DELETE_SUBSCR] = { .nuops = 1, .uops = { { DELETE_SUBSCR, 0, 0 } } }, [DELETE_SUBSCR] = { .nuops = 1, .uops = { { DELETE_SUBSCR, 0, 0 } } },
[CALL_INTRINSIC_1] = { .nuops = 1, .uops = { { CALL_INTRINSIC_1, 0, 0 } } }, [CALL_INTRINSIC_1] = { .nuops = 1, .uops = { { CALL_INTRINSIC_1, 0, 0 } } },
[CALL_INTRINSIC_2] = { .nuops = 1, .uops = { { CALL_INTRINSIC_2, 0, 0 } } }, [CALL_INTRINSIC_2] = { .nuops = 1, .uops = { { CALL_INTRINSIC_2, 0, 0 } } },
[RETURN_VALUE] = { .nuops = 2, .uops = { { _SAVE_CURRENT_IP, 7, -1 }, { _POP_FRAME, 0, 0 } } }, [RETURN_VALUE] = { .nuops = 1, .uops = { { _POP_FRAME, 0, 0 } } },
[RETURN_CONST] = { .nuops = 3, .uops = { { LOAD_CONST, 0, 0 }, { _SAVE_CURRENT_IP, 7, -1 }, { _POP_FRAME, 0, 0 } } }, [RETURN_CONST] = { .nuops = 2, .uops = { { LOAD_CONST, 0, 0 }, { _POP_FRAME, 0, 0 } } },
[GET_AITER] = { .nuops = 1, .uops = { { GET_AITER, 0, 0 } } }, [GET_AITER] = { .nuops = 1, .uops = { { GET_AITER, 0, 0 } } },
[GET_ANEXT] = { .nuops = 1, .uops = { { GET_ANEXT, 0, 0 } } }, [GET_ANEXT] = { .nuops = 1, .uops = { { GET_ANEXT, 0, 0 } } },
[GET_AWAITABLE] = { .nuops = 1, .uops = { { GET_AWAITABLE, 0, 0 } } }, [GET_AWAITABLE] = { .nuops = 1, .uops = { { GET_AWAITABLE, 0, 0 } } },

View file

@ -0,0 +1 @@
Remove unnecessary instruction pointer updates before returning from frames.

View file

@ -803,7 +803,6 @@ dummy_func(
} }
macro(RETURN_VALUE) = macro(RETURN_VALUE) =
_SAVE_CURRENT_IP + // Sets frame->prev_instr
_POP_FRAME; _POP_FRAME;
inst(INSTRUMENTED_RETURN_VALUE, (retval --)) { inst(INSTRUMENTED_RETURN_VALUE, (retval --)) {
@ -827,7 +826,6 @@ dummy_func(
macro(RETURN_CONST) = macro(RETURN_CONST) =
LOAD_CONST + LOAD_CONST +
_SAVE_CURRENT_IP + // Sets frame->prev_instr
_POP_FRAME; _POP_FRAME;
inst(INSTRUMENTED_RETURN_CONST, (--)) { inst(INSTRUMENTED_RETURN_CONST, (--)) {

View file

@ -987,36 +987,28 @@
TARGET(RETURN_VALUE) { TARGET(RETURN_VALUE) {
PyObject *retval; PyObject *retval;
// _SAVE_CURRENT_IP
{
TIER_ONE_ONLY
frame->prev_instr = next_instr - 1;
}
// _POP_FRAME
retval = stack_pointer[-1]; retval = stack_pointer[-1];
STACK_SHRINK(1); STACK_SHRINK(1);
{ assert(EMPTY());
assert(EMPTY()); #if TIER_ONE
#if TIER_ONE assert(frame != &entry_frame);
assert(frame != &entry_frame); #endif
#endif STORE_SP();
STORE_SP(); _Py_LeaveRecursiveCallPy(tstate);
_Py_LeaveRecursiveCallPy(tstate); // GH-99729: We need to unlink the frame *before* clearing it:
// GH-99729: We need to unlink the frame *before* clearing it: _PyInterpreterFrame *dying = frame;
_PyInterpreterFrame *dying = frame; frame = tstate->current_frame = dying->previous;
frame = tstate->current_frame = dying->previous; _PyEval_FrameClearAndPop(tstate, dying);
_PyEval_FrameClearAndPop(tstate, dying); frame->prev_instr += frame->return_offset;
frame->prev_instr += frame->return_offset; _PyFrame_StackPush(frame, retval);
_PyFrame_StackPush(frame, retval); LOAD_SP();
LOAD_SP(); LOAD_IP();
LOAD_IP(); #if LLTRACE && TIER_ONE
#if LLTRACE && TIER_ONE lltrace = maybe_lltrace_resume_frame(frame, &entry_frame, GLOBALS());
lltrace = maybe_lltrace_resume_frame(frame, &entry_frame, GLOBALS()); if (lltrace < 0) {
if (lltrace < 0) { goto exit_unwind;
goto exit_unwind;
}
#endif
} }
#endif
DISPATCH(); DISPATCH();
} }
@ -1049,11 +1041,6 @@
value = GETITEM(FRAME_CO_CONSTS, oparg); value = GETITEM(FRAME_CO_CONSTS, oparg);
Py_INCREF(value); Py_INCREF(value);
} }
// _SAVE_CURRENT_IP
{
TIER_ONE_ONLY
frame->prev_instr = next_instr - 1;
}
// _POP_FRAME // _POP_FRAME
retval = value; retval = value;
{ {

View file

@ -701,7 +701,9 @@ pop_jump_if_bool:
case OPARG_BOTTOM: // Second half of super-instr case OPARG_BOTTOM: // Second half of super-instr
oparg = orig_oparg & 0xF; oparg = orig_oparg & 0xF;
break; break;
case OPARG_SET_IP: // op==_SET_IP; oparg=next instr case OPARG_SET_IP: // uop=_SET_IP; oparg=next_instr-1
// The number of caches is smuggled in via offset:
assert(offset == _PyOpcode_Caches[_PyOpcode_Deopt[opcode]]);
oparg = INSTR_IP(instr + offset, code); oparg = INSTR_IP(instr + offset, code);
uop = _SET_IP; uop = _SET_IP;
break; break;
@ -850,11 +852,7 @@ remove_unneeded_uops(_PyUOpInstruction *trace, int trace_length)
bool need_ip = true; bool need_ip = true;
for (int pc = 0; pc < trace_length; pc++) { for (int pc = 0; pc < trace_length; pc++) {
int opcode = trace[pc].opcode; int opcode = trace[pc].opcode;
if (opcode == _SAVE_CURRENT_IP) { if (opcode == _SET_IP) {
// Special case: never remove preceding _SET_IP
last_set_ip = -1;
}
else if (opcode == _SET_IP) {
if (!need_ip && last_set_ip >= 0) { if (!need_ip && last_set_ip >= 0) {
trace[last_set_ip].opcode = NOP; trace[last_set_ip].opcode = NOP;
} }
@ -866,8 +864,8 @@ remove_unneeded_uops(_PyUOpInstruction *trace, int trace_length)
break; break;
} }
else { else {
// If opcode has ERROR or DEOPT, set need_up to true // If opcode has ERROR or DEOPT, set need_ip to true
if (_PyOpcode_opcode_metadata[opcode].flags & (HAS_ERROR_FLAG | HAS_DEOPT_FLAG)) { if (_PyOpcode_opcode_metadata[opcode].flags & (HAS_ERROR_FLAG | HAS_DEOPT_FLAG) || opcode == _PUSH_FRAME) {
need_ip = true; need_ip = true;
} }
} }