First half of reviews

Fidget-Spinner · Fidget-Spinner · commit 7d17741473d6 · 2025-10-21T21:21:29.000+01:00
diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h
@@ -757,7 +757,7 @@ struct _Py_unique_id_pool {
 
 typedef _Py_CODEUNIT *(*_PyJitEntryFuncPtr)(struct _PyExecutorObject *exec, _PyInterpreterFrame *frame, _PyStackRef *stack_pointer, PyThreadState *tstate);
 
-typedef struct _PyJitState {
+typedef struct _PyJitTracerState {
     int jit_tracer_code_max_size;
     int jit_tracer_code_curr_size;
     _PyBloomFilter jit_tracer_dependencies;
@@ -772,7 +772,7 @@ typedef struct _PyJitState {
     PyFunctionObject *jit_tracer_initial_func; // Strong
     struct _PyExitData *jit_tracer_previous_exit;
     _PyInterpreterFrame *jit_tracer_current_frame;
-} _PyJitState;
+} _PyJitTracerState;
 
 /* PyInterpreterState holds the global state for one of the runtime's
    interpreters.  Typically the initial (main) interpreter is the only one.
@@ -949,7 +949,7 @@ struct _is {
     struct types_state types;
     struct callable_cache callable_cache;
     PyObject *common_consts[NUM_COMMON_CONSTANTS];
-    _PyJitState jit_state;
+    _PyJitTracerState jit_state;
     bool jit;
     bool compiling;
     struct _PyExecutorObject *executor_list_head;
diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h
diff --git a/Include/internal/pycore_optimizer.h b/Include/internal/pycore_optimizer.h
@@ -37,7 +37,6 @@ typedef struct {
 typedef struct _PyExitData {
     uint32_t target;
     uint16_t index;
-    char is_dynamic;
     _Py_BackoffCounter temperature;
     struct _PyExecutorObject *executor;
 } _PyExitData;
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
@@ -1378,7 +1378,7 @@ dummy_func(
                 if (err == 0) {
                     assert(retval_o != NULL);
                     JUMPBY(oparg);
-                    RECORD_JUMP_TAKEN();
+                    RECORD_DYNAMIC_JUMP_TAKEN();
                 }
                 else {
                     PyStackRef_CLOSE(v);
@@ -3241,7 +3241,7 @@ dummy_func(
                 }
                 // Jump forward by oparg and skip the following END_FOR
                 JUMPBY(oparg + 1);
-                RECORD_JUMP_TAKEN();
+                RECORD_DYNAMIC_JUMP_TAKEN();
                 DISPATCH();
             }
             next = item;
@@ -3303,7 +3303,7 @@ dummy_func(
                 null_or_index = PyStackRef_TagInt(-1);
                 /* Jump forward oparg, then skip following END_FOR instruction */
                 JUMPBY(oparg + 1);
-                RECORD_JUMP_TAKEN();
+                RECORD_DYNAMIC_JUMP_TAKEN();
                 DISPATCH();
             }
 #endif
@@ -3381,7 +3381,7 @@ dummy_func(
                 null_or_index = PyStackRef_TagInt(-1);
                 /* Jump forward oparg, then skip following END_FOR instruction */
                 JUMPBY(oparg + 1);
-                RECORD_JUMP_TAKEN();
+                RECORD_DYNAMIC_JUMP_TAKEN();
                 DISPATCH();
             }
         }
@@ -3426,7 +3426,7 @@ dummy_func(
             if (r->len <= 0) {
                 // Jump over END_FOR instruction.
                 JUMPBY(oparg + 1);
-                RECORD_JUMP_TAKEN();
+                RECORD_DYNAMIC_JUMP_TAKEN();
                 DISPATCH();
             }
         }
@@ -5011,7 +5011,7 @@ dummy_func(
             LOAD_IP(frame->return_offset);
             #endif
             #if TIER_TWO
-            frame->instr_ptr += (frame->return_offset);
+            TIER2_STORE_IP(frame->return_offset);
             #endif
             RELOAD_STACK();
             res = PyStackRef_FromPyObjectStealMortal((PyObject *)gen);
@@ -5278,7 +5278,6 @@ dummy_func(
 
         tier2 op(_EXIT_TRACE, (exit_p/4 --)) {
             _PyExitData *exit = (_PyExitData *)exit_p;
-            assert(!exit->is_dynamic);
         #if defined(Py_DEBUG) && !defined(_Py_JIT)
             _Py_CODEUNIT *target = _PyFrame_GetBytecode(frame) + exit->target;
             OPT_HIST(trace_uop_execution_counter, trace_run_length_hist);
diff --git a/Python/ceval_macros.h b/Python/ceval_macros.h
@@ -130,7 +130,7 @@
 #endif
 
 #define TRACING_JUMP_TO_LABEL(label) \
-    RECORD_JUMP_TAKEN() \
+    RECORD_DYNAMIC_JUMP_TAKEN() \
     RECORD_TRACE_NO_DISPATCH() \
     assert(!IS_JIT_TRACING()); \
     JUMP_TO_LABEL(label);
@@ -381,7 +381,7 @@ GETITEM(PyObject *v, Py_ssize_t i) {
 #define RECORD_BRANCH_TAKEN(bitset, flag)
 #endif
 
-#define RECORD_JUMP_TAKEN() _jump_taken = 1;
+#define RECORD_DYNAMIC_JUMP_TAKEN() _jump_taken = 1;
 
 #define UNBOUNDLOCAL_ERROR_MSG \
     "cannot access local variable '%s' where it is not associated with a value"
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
diff --git a/Python/generated_tracer_cases.c.h b/Python/generated_tracer_cases.c.h
diff --git a/Python/optimizer.c b/Python/optimizer.c
@@ -141,13 +141,7 @@ _PyOptimizer_Optimize(
         interp->compiling = false;
         return 0;
     }
-    // We are the only one still holding a reference to this code object that
-    // is practically dead.
-    if (_PyObject_IsUniquelyReferenced((PyObject *)code) || _PyObject_IsUniquelyReferenced((PyObject *)tstate->interp->jit_state.jit_tracer_initial_func)) {
-        interp->compiling = false;
-        return 0;
-    }
-    // One of our depencies while tracing was invalidated. Not worth compiling.
+    // One of our dependencies while tracing was invalidated. Not worth compiling.
     if (!tstate->interp->jit_state.jit_tracer_dependencies_still_valid) {
         interp->compiling = false;
         return 0;
@@ -603,20 +597,21 @@ _PyJIT_translate_single_bytecode_to_trace(
     }
 #endif
 
+    if (!tstate->interp->jit_state.jit_tracer_dependencies_still_valid) {
+        goto done;
+    }
+
     DPRINTF(2, "%p %d: %s(%d) %d\n", old_code, target, _PyOpcode_OpName[opcode], oparg, progress_needed);
 
-    bool needs_guard_ip = _PyOpcode_NeedsGuardIp[opcode] &&
-        !(opcode == FOR_ITER_RANGE || opcode == FOR_ITER_LIST || opcode == FOR_ITER_TUPLE) &&
-        !(opcode == JUMP_BACKWARD_NO_INTERRUPT || opcode == JUMP_BACKWARD || opcode == JUMP_BACKWARD_JIT) &&
-        !(opcode == POP_JUMP_IF_TRUE || opcode == POP_JUMP_IF_FALSE || opcode == POP_JUMP_IF_NONE || opcode == POP_JUMP_IF_NOT_NONE);
+    bool needs_guard_ip = _PyOpcode_NeedsGuardIp[opcode];
 
     // Strange control-flow, unsupported opcode, etc.
     if (jump_taken ||
         // This happens when a recursive call happens that we can't trace. Such as Python -> C -> Python calls
         // If we haven't guarded the IP, then it's untraceable.
         (frame != tstate->interp->jit_state.jit_tracer_current_frame && !needs_guard_ip) ||
         (oparg > 0xFFFF) ||
-        // TODO (gh-140277): The constituent use one extra stack slot. So we need to check for heaedroom.
+        // TODO (gh-140277): The constituent use one extra stack slot. So we need to check for headroom.
         (opcode == BINARY_OP_SUBSCR_GETITEM && old_stack_level + 1 > old_code->co_stacksize)||
         // Exception stuff, could be handled in the future maybe?
         opcode == WITH_EXCEPT_START || opcode == RERAISE || opcode == CLEANUP_THROW || opcode == PUSH_EXC_INFO ||
@@ -1155,7 +1150,6 @@ make_executor_from_uops(_PyUOpInstruction *buffer, int length, const _PyBloomFil
             _PyExitData *exit = &executor->exits[next_exit];
             exit->target = buffer[i].target;
             dest->operand0 = (uint64_t)exit;
-            exit->is_dynamic = (char)(opcode == _DYNAMIC_EXIT);
             next_exit--;
         }
     }
@@ -1561,20 +1555,15 @@ _Py_Executors_InvalidateDependency(PyInterpreterState *interp, void *obj, int is
 }
 
 void
-_Py_JITTracer_InvalidateDependency(PyThreadState *old_tstate, void *obj)
+_Py_JITTracer_InvalidateDependency(PyThreadState *tstate, void *obj)
 {
     _PyBloomFilter obj_filter;
     _Py_BloomFilter_Init(&obj_filter);
     _Py_BloomFilter_Add(&obj_filter, obj);
 
-    PyInterpreterState *interp = old_tstate->interp;
-
-    _Py_FOR_EACH_TSTATE_UNLOCKED(interp, tstate) {
-        if (bloom_filter_may_contain(&tstate->interp->jit_state.jit_tracer_dependencies, &obj_filter))
-        {
-            tstate->interp->jit_state.jit_tracer_dependencies_still_valid = false;
-        }
-
+    if (bloom_filter_may_contain(&tstate->interp->jit_state.jit_tracer_dependencies, &obj_filter))
+    {
+        tstate->interp->jit_state.jit_tracer_dependencies_still_valid = false;
     }
 }
 /* Invalidate all executors */
diff --git a/Python/optimizer_analysis.c b/Python/optimizer_analysis.c
@@ -528,10 +528,14 @@ _Py_uop_analyze_and_optimize(
 {
     OPT_STAT_INC(optimizer_attempts);
 
-    optimize_uops(
+    length = optimize_uops(
          initial_func, buffer,
          length, curr_stacklen, dependencies);
 
+    if (length == 0) {
+        return length;
+    }
+
     assert(length > 0);
 
     length = remove_unneeded_uops(buffer, length);
diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py
@@ -937,7 +937,7 @@ def compute_properties(op: parser.CodeDef) -> Properties:
         no_save_ip=no_save_ip,
         tier=tier_variable(op),
         needs_prev=variable_used(op, "prev_instr"),
-        needs_guard_ip=variable_used(op, "JUMPBY") or variable_used(op, "LLTRACE_RESUME_FRAME"),
+        needs_guard_ip=variable_used(op, "TIER2_STORE_IP") or variable_used(op, "LLTRACE_RESUME_FRAME") or variable_used(op, "DISPATCH_INLINED"),
     )
 
 def expand(items: list[StackItem], oparg: int) -> list[StackItem]:
diff --git a/Tools/cases_generator/generators_common.py b/Tools/cases_generator/generators_common.py
@@ -128,7 +128,7 @@ def __init__(self, out: CWriter, labels: dict[str, Label], cannot_escape: bool =
             "DISPATCH": self.dispatch,
             "INSTRUCTION_SIZE": self.instruction_size,
             "stack_pointer": self.stack_pointer,
-            "RECORD_JUMP_TAKEN": self.record_jump_taken,
+            "RECORD_DYNAMIC_JUMP_TAKEN": self.record_dynamic_jump_taken,
         }
         self.out = out
         self.labels = labels
@@ -476,7 +476,7 @@ def instruction_size(self,
         self.out.emit(f" {uop.instruction_size}u ")
         return True
 
-    def record_jump_taken(
+    def record_dynamic_jump_taken(
         self,
         tkn: Token,
         tkn_iter: TokenIterator,
diff --git a/Tools/cases_generator/tracer_generator.py b/Tools/cases_generator/tracer_generator.py
@@ -96,7 +96,7 @@ def dispatch_same_oparg(
         emit_to(self.out, tkn_iter, "SEMI")
         return False
 
-    def record_jump_taken(
+    def record_dynamic_jump_taken(
         self,
         tkn: Token,
         tkn_iter: TokenIterator,
