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

diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h
index 926c0041c34c28ba2d029ddee13d01d07648540d..938597dadb2897873367f121a0566549f1d858c7 100644 (file)
--- a/Include/internal/pycore_opcode_metadata.h
+++ b/Include/internal/pycore_opcode_metadata.h
@@ -1644,8 +1644,8 @@ const struct opcode_macro_expansion _PyOpcode_macro_expansion[OPCODE_MACRO_EXPAN
     [DELETE_SUBSCR] = { .nuops = 1, .uops = { { DELETE_SUBSCR, 0, 0 } } },
     [CALL_INTRINSIC_1] = { .nuops = 1, .uops = { { CALL_INTRINSIC_1, 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_CONST] = { .nuops = 3, .uops = { { LOAD_CONST, 0, 0 }, { _SAVE_CURRENT_IP, 7, -1 }, { _POP_FRAME, 0, 0 } } },
+    [RETURN_VALUE] = { .nuops = 1, .uops = { { _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_ANEXT] = { .nuops = 1, .uops = { { GET_ANEXT, 0, 0 } } },
     [GET_AWAITABLE] = { .nuops = 1, .uops = { { GET_AWAITABLE, 0, 0 } } },

diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-10-16-15-51-37.gh-issue-109214.-RGTFH.rst b/Misc/NEWS.d/next/Core and Builtins/2023-10-16-15-51-37.gh-issue-109214.-RGTFH.rst
new file mode 100644 (file)
index 0000000..c24f18c
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2023-10-16-15-51-37.gh-issue-109214.-RGTFH.rst
@@ -0,0 +1 @@
+Remove unnecessary instruction pointer updates before returning from frames.

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index d7e2ecdd24dceef30afb98b104e84110ce4681b6..2ff3805c563de11f228d2a65d306fa32a310dfb9 100644 (file)
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -803,7 +803,6 @@ dummy_func(
         }
 
         macro(RETURN_VALUE) =
-            _SAVE_CURRENT_IP +  // Sets frame->prev_instr
             _POP_FRAME;
 
         inst(INSTRUMENTED_RETURN_VALUE, (retval --)) {
@@ -827,7 +826,6 @@ dummy_func(
 
         macro(RETURN_CONST) =
             LOAD_CONST +
-            _SAVE_CURRENT_IP +  // Sets frame->prev_instr
             _POP_FRAME;
 
         inst(INSTRUMENTED_RETURN_CONST, (--)) {

diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index 6fbe80fe03a128e2234347a722537abb2580b48a..506c02a6f74728c0e44979a883f0b72271a3029e 100644 (file)
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -987,36 +987,28 @@
 
         TARGET(RETURN_VALUE) {
             PyObject *retval;
-            // _SAVE_CURRENT_IP
-            {
-                TIER_ONE_ONLY
-                frame->prev_instr = next_instr - 1;
-            }
-            // _POP_FRAME
             retval = stack_pointer[-1];
             STACK_SHRINK(1);
-            {
-                assert(EMPTY());
-                #if TIER_ONE
-                assert(frame != &entry_frame);
-                #endif
-                STORE_SP();
-                _Py_LeaveRecursiveCallPy(tstate);
-                // GH-99729: We need to unlink the frame *before* clearing it:
-                _PyInterpreterFrame *dying = frame;
-                frame = tstate->current_frame = dying->previous;
-                _PyEval_FrameClearAndPop(tstate, dying);
-                frame->prev_instr += frame->return_offset;
-                _PyFrame_StackPush(frame, retval);
-                LOAD_SP();
-                LOAD_IP();
-    #if LLTRACE && TIER_ONE
-                lltrace = maybe_lltrace_resume_frame(frame, &entry_frame, GLOBALS());
-                if (lltrace < 0) {
-                    goto exit_unwind;
-                }
-    #endif
+            assert(EMPTY());
+            #if TIER_ONE
+            assert(frame != &entry_frame);
+            #endif
+            STORE_SP();
+            _Py_LeaveRecursiveCallPy(tstate);
+            // GH-99729: We need to unlink the frame *before* clearing it:
+            _PyInterpreterFrame *dying = frame;
+            frame = tstate->current_frame = dying->previous;
+            _PyEval_FrameClearAndPop(tstate, dying);
+            frame->prev_instr += frame->return_offset;
+            _PyFrame_StackPush(frame, retval);
+            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();
         }
 
@@ -1049,11 +1041,6 @@
                 value = GETITEM(FRAME_CO_CONSTS, oparg);
                 Py_INCREF(value);
             }
-            // _SAVE_CURRENT_IP
-            {
-                TIER_ONE_ONLY
-                frame->prev_instr = next_instr - 1;
-            }
             // _POP_FRAME
             retval = value;
             {

diff --git a/Python/optimizer.c b/Python/optimizer.c
index 8d19de220d3d3d58201183d2d8679281f577e612..3dfd4f3df66db0231ae886d1d23bc09c8d7d6b9b 100644 (file)
--- a/Python/optimizer.c
+++ b/Python/optimizer.c
@@ -701,7 +701,9 @@ pop_jump_if_bool:
                             case OPARG_BOTTOM:  // Second half of super-instr
                                 oparg = orig_oparg & 0xF;
                                 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);
                                 uop = _SET_IP;
                                 break;
@@ -850,11 +852,7 @@ remove_unneeded_uops(_PyUOpInstruction *trace, int trace_length)
     bool need_ip = true;
     for (int pc = 0; pc < trace_length; pc++) {
         int opcode = trace[pc].opcode;
-        if (opcode == _SAVE_CURRENT_IP) {
-            // Special case: never remove preceding _SET_IP
-            last_set_ip = -1;
-        }
-        else if (opcode == _SET_IP) {
+        if (opcode == _SET_IP) {
             if (!need_ip && last_set_ip >= 0) {
                 trace[last_set_ip].opcode = NOP;
             }
@@ -866,8 +864,8 @@ remove_unneeded_uops(_PyUOpInstruction *trace, int trace_length)
             break;
         }
         else {
-            // If opcode has ERROR or DEOPT, set need_up to true
-            if (_PyOpcode_opcode_metadata[opcode].flags & (HAS_ERROR_FLAG | HAS_DEOPT_FLAG)) {
+            // If opcode has ERROR or DEOPT, set need_ip to true
+            if (_PyOpcode_opcode_metadata[opcode].flags & (HAS_ERROR_FLAG | HAS_DEOPT_FLAG) || opcode == _PUSH_FRAME) {
                 need_ip = true;
             }
         }