Most suggestions from Mark's code review

- Remove CHECK_EVAL_BREAKER() from top of Tier 2 loop - Make Tier 2 default case Py_UNREACHABLE() in non-debug mode - GOTO_TIER_TWO() macro instead of `goto enter_tier_two` - Inline ip_offset (Tier 2 LOAD_IP() is now a no-op) - Move #define/#under TIER_ONE/TIER_TWO into generated .c.h files - ceval_macros.h defines Tier 1 macros, Tier 2 is inlined in ceval.c
python · gvanrossum · Nov 1, 2023 · Oct 27, 2023 · Oct 27, 2023 · Oct 27, 2023
commit 75605c7077da1e697615f4126daecbd0494b8f72
@@ -2314,7 +2314,7 @@ dummy_func(
             Py_INCREF(executor);
             if (executor->execute == _PyUopExecute) {
                 self = (_PyUOpExecutorObject *)executor;
-                goto enter_tier_two;
+                GOTO_TIER_TWO();
             }
             frame = executor->execute(executor, frame, stack_pointer);
             if (frame == NULL) {
@@ -3942,7 +3942,8 @@ dummy_func(
 
         op(_SET_IP, (--)) {
             TIER_TWO_ONLY
-            frame->instr_ptr = ip_offset + oparg;
+            // TODO: Put the code pointer in `operand` to avoid indirection via `frame`
+            frame->instr_ptr = _PyCode_CODE(_PyFrame_GetCode(frame)) + oparg;
         }
 
         op(_SAVE_RETURN_OFFSET, (--)) {

@@ -611,10 +611,8 @@ PyEval_EvalFrameEx(PyFrameObject *f, int throwflag)
     return _PyEval_EvalFrame(tstate, f->f_frame, throwflag);
 }
 
-#define TIER_ONE 1
 #include "ceval_macros.h"
 
-
 int _Py_CheckRecursiveCallPy(
     PyThreadState *tstate)
 {
@@ -952,8 +950,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
 enter_tier_two:
 
 #undef LOAD_IP
-#define LOAD_IP(UNUSED) \
-do { ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive; } while (0)
+#define LOAD_IP(UNUSED) (void)0
 
 #undef GOTO_ERROR
 #define GOTO_ERROR(LABEL) goto LABEL ## _tier_two
@@ -984,10 +981,7 @@ do { ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive; } wh
     #define DPRINTF(level, ...)
 #endif
 
-    CHECK_EVAL_BREAKER();
-
     OPT_STAT_INC(traces_executed);
-    _Py_CODEUNIT *ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive;
     _PyUOpInstruction *next_uop = self->trace;
     uint64_t operand;
 #ifdef Py_STATS
@@ -1013,16 +1007,18 @@ do { ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive; } wh
 
         switch (opcode) {
 
-#undef TIER_ONE
-#define TIER_TWO 2
 #include "executor_cases.c.h"
 
             default:
+#ifdef Py_DEBUG
             {
                 fprintf(stderr, "Unknown uop %d, oparg %d, operand %" PRIu64 "\n",
                         opcode, oparg, operand);
                 Py_FatalError("Unknown uop");
             }
+#else
+            Py_UNREACHABLE();
+#endif
 
         }
     }

@@ -377,36 +377,21 @@ static inline void _Py_LeaveRecursiveCallPy(PyThreadState *tstate)  {
 
 /* Implementation of "macros" that modify the instruction pointer,
  * stack pointer, or frame pointer.
- * These need to treated differently by tier 1 and 2. */
-
-#if TIER_ONE
+ * These need to treated differently by tier 1 and 2. 
+ * The Tier 1 version is here; Tier 2 is inlined in ceval.c. */
 
 #define LOAD_IP(OFFSET) do { \
         next_instr = frame->instr_ptr + (OFFSET); \
     } while (0)
 
-#define STORE_SP() \
-_PyFrame_SetStackPointer(frame, stack_pointer)
-
-#define LOAD_SP() \
-stack_pointer = _PyFrame_GetStackPointer(frame);
-
-#endif
-
-
-#if TIER_TWO
-
-#define LOAD_IP(UNUSED) \
-do { ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive; } while (0)
+/* There's no STORE_IP(), it's inlined by the code generator. */
 
 #define STORE_SP() \
 _PyFrame_SetStackPointer(frame, stack_pointer)
 
 #define LOAD_SP() \
 stack_pointer = _PyFrame_GetStackPointer(frame);
 
-#endif
-
-
-
+/* Tier-switching macros. */
 
+#define GOTO_TIER_TWO() goto enter_tier_two;
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
@@ -758,6 +758,12 @@ def write_instructions(
 
             self.write_provenance_header()
 
+            self.out.write_raw("\n")
+            self.out.write_raw("#ifdef TIER_TWO\n")
+            self.out.write_raw("    #error \"This file is for Tier 1 only\"\n")
+            self.out.write_raw("#endif\n")
+            self.out.write_raw("#define TIER_ONE 1\n")
+
             # Write and count instructions of all kinds
             n_macros = 0
             for thing in self.everything:
@@ -773,6 +779,9 @@ def write_instructions(
                     case _:
                         assert_never(thing)
 
+            self.out.write_raw("\n")
+            self.out.write_raw("#undef TIER_ONE\n")
+
         print(
             f"Wrote {n_macros} cases to {output_filename}",
             file=sys.stderr,
@@ -786,6 +795,13 @@ def write_executor_instructions(
         with open(executor_filename, "w") as f:
             self.out = Formatter(f, 8, emit_line_directives)
             self.write_provenance_header()
+
+            self.out.write_raw("\n")
+            self.out.write_raw("#ifdef TIER_ONE\n")
+            self.out.write_raw("    #error \"This file is for Tier 2 only\"\n")
+            self.out.write_raw("#endif\n")
+            self.out.write_raw("#define TIER_TWO 2\n")
+
             for instr in self.instrs.values():
                 if instr.is_viable_uop():
                     n_uops += 1
@@ -795,6 +811,10 @@ def write_executor_instructions(
                         if instr.check_eval_breaker:
                             self.out.emit("CHECK_EVAL_BREAKER();")
                         self.out.emit("break;")
+
+            self.out.write_raw("\n")
+            self.out.write_raw("#undef TIER_TWO\n")
+
         print(
             f"Wrote {n_uops} cases to {executor_filename}",
             file=sys.stderr,