Introduce SAVE_CURRENT_IP uop per Mark's proposal

gvanrossum · Aug 5, 2023 · Aug 9, 2023 · Aug 9, 2023 · Aug 10, 2023 · Aug 10, 2023
commit 05af84805a04747c1de7a5b843041b8d55c31bee
diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h
diff --git a/Python/abstract_interp_cases.c.h b/Python/abstract_interp_cases.c.h
@@ -2991,7 +2991,7 @@ dummy_func(
             // Eventually this should be the only occurrence of this code.
             frame->return_offset = 0;
             assert(tstate->interp->eval_frame == NULL);
-            SAVE_FRAME_STATE();  // Signals to the code generator
+            _PyFrame_SetStackPointer(frame, stack_pointer);
             new_frame->previous = frame;
             CALL_STAT_INC(inlined_py_calls);
             #if TIER_ONE
@@ -3013,6 +3013,7 @@ dummy_func(
             _CHECK_STACK_SPACE +
             _INIT_CALL_PY_EXACT_ARGS +
             SAVE_IP +  // Tier 2 only; special-cased oparg
+            SAVE_CURRENT_IP +  // Sets frame->prev_instr
             _PUSH_FRAME;
 
         inst(CALL_PY_WITH_DEFAULTS, (unused/1, func_version/2, callable, self_or_null, args[oparg] -- unused)) {
@@ -3775,6 +3776,16 @@ dummy_func(
             frame->prev_instr = ip_offset + oparg;
         }
 
+        op(SAVE_CURRENT_IP, (--)) {
+            #if TIER_ONE
+            frame->prev_instr = next_instr - 1;
+            #endif
+            #if TIER_TWO
+            // Relies on a preceding SAVE_IP
+            frame->prev_instr--;
+            #endif
+        }
+
         op(EXIT_TRACE, (--)) {
             frame->prev_instr--;  // Back up to just before destination
             _PyFrame_SetStackPointer(frame, stack_pointer);

@@ -103,16 +103,11 @@
         DISPATCH_GOTO(); \
     }
 
-#define SAVE_FRAME_STATE() \
-    do { \
-        frame->prev_instr = next_instr - 1; \
-        _PyFrame_SetStackPointer(frame, stack_pointer); \
-    } while (0)
-
 #define DISPATCH_INLINED(NEW_FRAME)                     \
     do {                                                \
         assert(tstate->interp->eval_frame == NULL);     \
-        SAVE_FRAME_STATE();                             \
+        _PyFrame_SetStackPointer(frame, stack_pointer); \
+        frame->prev_instr = next_instr - 1;             \
         (NEW_FRAME)->previous = frame;                  \
         frame = cframe.current_frame = (NEW_FRAME);     \
         CALL_STAT_INC(inlined_py_calls);                \

diff --git a/Python/executor.c b/Python/executor.c
@@ -30,14 +30,6 @@
 #undef ENABLE_SPECIALIZATION
 #define ENABLE_SPECIALIZATION 0
 
-#undef SAVE_FRAME_STATE
-#define SAVE_FRAME_STATE() \
-    do { \
-        /* Assume preceding SAVE_IP has set frame->prev_instr */ \
-        frame->prev_instr--; \
-        _PyFrame_SetStackPointer(frame, stack_pointer); \
-    } while (0)
-
 
 _PyInterpreterFrame *
 _PyUopExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject **stack_pointer)

diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h
diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
@@ -92,7 +92,7 @@ def variable_used_unspecialized(node: parsing.Node, name: str) -> bool:
             if text == "#if":
                 if (
                     i + 1 < len(node.tokens)
-                    and node.tokens[i + 1].text == "ENABLE_SPECIALIZATION"
+                    and node.tokens[i + 1].text in ("ENABLE_SPECIALIZATION", "TIER_ONE")
                 ):
                     skipping = True
             elif text in ("#else", "#endif"):

@@ -60,7 +60,6 @@ class Instruction:
 
     # Computed by constructor
     always_exits: str  # If the block always exits, its last line; else ""
-    save_frame_state: bool  # Whether the instruction uses SAVE_FRAME_STATE()
     has_deopt: bool
     cache_offset: int
     cache_effects: list[parsing.CacheEffect]
@@ -84,7 +83,6 @@ def __init__(self, inst: parsing.InstDef):
             self.block
         )
         self.always_exits = always_exits(self.block_text)
-        self.save_frame_state = variable_used(self.inst, "SAVE_FRAME_STATE")
         self.has_deopt = variable_used(self.inst, "DEOPT_IF")
         self.cache_effects = [
             effect for effect in inst.inputs if isinstance(effect, parsing.CacheEffect)
@@ -122,7 +120,7 @@ def __init__(self, inst: parsing.InstDef):
     def is_viable_uop(self) -> bool:
         """Whether this instruction is viable as a uop."""
         dprint: typing.Callable[..., None] = lambda *args, **kwargs: None
-        if "PUSH_FRAME" in self.name:
+        if "FRAME" in self.name:
             dprint = print
 
         if self.name == "EXIT_TRACE":

@@ -324,10 +324,10 @@ def write_macro_instr(
             out.emit(f"PREDICTED({mac.name});")
         out.static_assert_family_size(mac.name, family, mac.cache_offset)
         try:
-            write_components(parts, out, TIER_ONE, mac.cache_offset)
+            next_instr_is_set = write_components(parts, out, TIER_ONE, mac.cache_offset)
         except AssertionError as err:
             raise AssertionError(f"Error writing macro {mac.name}") from err
-        if not parts[-1].instr.always_exits and not parts[-1].instr.save_frame_state:
+        if not parts[-1].instr.always_exits and not next_instr_is_set:
             if mac.cache_offset:
                 out.emit(f"next_instr += {mac.cache_offset};")
             out.emit("DISPATCH();")
@@ -338,7 +338,7 @@ def write_components(
     out: Formatter,
     tier: Tiers,
     cache_offset: int,
-) -> None:
+) -> bool:
     managers = get_managers(parts)
 
     all_vars: dict[str, StackEffect] = {}
@@ -359,6 +359,7 @@ def write_components(
     for name, eff in all_vars.items():
         out.declare(eff, None)
 
+    next_instr_is_set = False
     for mgr in managers:
         if len(parts) > 1:
             out.emit(f"// {mgr.instr.name}")
@@ -379,11 +380,14 @@ def write_components(
                     poke.as_stack_effect(lax=True),
                 )
 
-        if mgr.instr.save_frame_state:
+        if mgr.instr.name == "_PUSH_FRAME":
             # Adjust stack to min_offset (input effects materialized)
             out.stack_adjust(mgr.min_offset.deep, mgr.min_offset.high)
             # Use clone() since adjust_inverse() mutates final_offset
             mgr.adjust_inverse(mgr.final_offset.clone())
+
+        if mgr.instr.name == "SAVE_CURRENT_IP":
+            next_instr_is_set = True
             if cache_offset:
                 out.emit(f"next_instr += {cache_offset};")
 
@@ -393,7 +397,7 @@ def write_components(
             with out.block(""):
                 mgr.instr.write_body(out, -4, mgr.active_caches, tier)
 
-        if mgr is managers[-1] and not mgr.instr.save_frame_state:
+        if mgr is managers[-1] and not next_instr_is_set:
             # TODO: Explain why this adjustment is needed.
             out.stack_adjust(mgr.final_offset.deep, mgr.final_offset.high)
             # Use clone() since adjust_inverse() mutates final_offset
@@ -406,6 +410,8 @@ def write_components(
                     poke.effect,
                 )
 
+    return next_instr_is_set
+
 
 def write_single_instr_for_abstract_interp(
     instr: Instruction, out: Formatter