Skip to content

Commit 37f0530

Browse files
Fidget-SpinnerFidget-Spinner
committed
Optimize NULL checks away completely in property tracking
1 parent ccbe41e commit 37f0530

File tree

9 files changed

+68
-122
lines changed

9 files changed

+68
-122
lines changed

‎Include/internal/pycore_optimizer_types.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ typedef struct {
116116
typedef struct _jit_opt_descr {
117117
uint8_t tag;
118118
uint8_t num_descrs;
119-
uint16_t last_modified_index; // Index in out_buffer when this object was last modified
119+
// Index in out_buffer when this object was last escaped
120+
uint16_t last_escape_index;
120121
uint32_t type_version;
121122
JitOptDescrMapping *descrs;
122123
} JitOptDescrObject;

‎Include/internal/pycore_uop_metadata.h‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Modules/_testinternalcapi/test_cases.c.h‎

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Python/bytecodes.c‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2677,8 +2677,7 @@ dummy_func(
26772677
STAT_INC(STORE_ATTR, hit);
26782678
assert(_PyObject_GetManagedDict(owner_o) == NULL);
26792679
PyObject **value_ptr = (PyObject**)(((char *)owner_o) + offset);
2680-
PyObject *old_value = *value_ptr;
2681-
DEOPT_IF(old_value != NULL);
2680+
assert(*value_ptr == NULL);
26822681
FT_ATOMIC_STORE_PTR_RELEASE(*value_ptr, PyStackRef_AsPyObjectSteal(value));
26832682
PyDictValues *values = _PyObject_InlineValues(owner_o);
26842683
Py_ssize_t index = value_ptr - values->values;
@@ -2756,8 +2755,7 @@ dummy_func(
27562755
DEOPT_IF(!LOCK_OBJECT(owner_o));
27572756
char *addr = (char *)owner_o + index;
27582757
STAT_INC(STORE_ATTR, hit);
2759-
PyObject *old_value = *(PyObject **)addr;
2760-
DEOPT_IF(old_value != NULL);
2758+
assert(*(PyObject **)addr == NULL);
27612759
FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, PyStackRef_AsPyObjectSteal(value));
27622760
UNLOCK_OBJECT(owner_o);
27632761
INPUTS_DEAD();

‎Python/executor_cases.c.h‎

Lines changed: 8 additions & 62 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Python/generated_cases.c.h‎

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Python/optimizer_analysis.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -526,9 +526,9 @@ optimize_uops(
526526
if (ctx->out_buffer.next == out_ptr) {
527527
*(ctx->out_buffer.next++) = *this_instr;
528528
}
529-
// Track escapes - but skip when from init shim frame, since self hasn't escaped yet
530529
bool is_init_shim = CURRENT_FRAME_IS_INIT_SHIM();
531-
if ((_PyUop_Flags[out_ptr->opcode] & HAS_ESCAPES_FLAG) && !is_init_shim)
530+
// Track escapes
531+
if (_PyUop_Flags[out_ptr->opcode] & HAS_ESCAPES_FLAG)
532532
{
533533
ctx->last_escape_index = uop_buffer_length(&ctx->out_buffer) - 1;
534534
}

‎Python/optimizer_symbols.c‎

Lines changed: 45 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -980,7 +980,7 @@ _Py_uop_sym_new_descr_object(JitOptContext *ctx, unsigned int type_version)
980980
res->descr.num_descrs = 0;
981981
res->descr.descrs = NULL;
982982
res->descr.type_version = type_version;
983-
res->descr.last_modified_index = uop_buffer_length(&ctx->out_buffer);
983+
res->descr.last_escape_index = uop_buffer_length(&ctx->out_buffer);
984984
return PyJitRef_Wrap(res);
985985
}
986986

@@ -995,7 +995,7 @@ _Py_uop_sym_get_attr(JitOptContext *ctx, JitOptRef ref, uint16_t slot_index)
995995
// 1. Symbol has mappings allocated
996996
// 2. No escape has occurred since last modification (state is fresh)
997997
if (sym->descr.descrs != NULL &&
998-
sym->descr.last_modified_index >= ctx->last_escape_index)
998+
sym->descr.last_escape_index >= ctx->last_escape_index)
999999
{
10001000
for (int i = 0; i < sym->descr.num_descrs; i++) {
10011001
if (sym->descr.descrs[i].slot_index == slot_index) {
@@ -1026,57 +1026,56 @@ JitOptRef
10261026
_Py_uop_sym_set_attr(JitOptContext *ctx, JitOptRef ref, uint16_t slot_index, JitOptRef value)
10271027
{
10281028
JitOptSymbol *sym = PyJitRef_Unwrap(ref);
1029-
int curr_index = uop_buffer_length(&ctx->out_buffer);
1030-
10311029
switch (sym->tag) {
1032-
case JIT_SYM_DESCR_TAG:
1033-
break;
1034-
default:
1035-
return _Py_uop_sym_new_not_null(ctx);
1036-
}
1037-
1038-
// Check escape
1039-
if (sym->descr.last_modified_index < ctx->last_escape_index) {
1040-
sym->descr.num_descrs = 0;
1041-
}
1030+
case JIT_SYM_DESCR_TAG: {
1031+
// Check escape
1032+
if (sym->descr.last_escape_index < ctx->last_escape_index) {
1033+
printf("ESCAPING %d %d\n", sym->descr.last_escape_index, ctx->last_escape_index);
1034+
sym->descr.num_descrs = 0;
1035+
return _Py_uop_sym_new_unknown(ctx);
1036+
}
10421037

1043-
// Get old value before updating
1044-
JitOptRef old_value = PyJitRef_NULL;
1045-
if (sym->descr.descrs != NULL) {
1046-
for (int i = 0; i < sym->descr.num_descrs; i++) {
1047-
if (sym->descr.descrs[i].slot_index == slot_index) {
1048-
old_value = PyJitRef_Wrap(allocation_base(ctx) + sym->descr.descrs[i].symbol);
1049-
break;
1038+
// Get old value before updating
1039+
JitOptRef old_value = _Py_uop_sym_new_unknown(ctx);
1040+
if (sym->descr.descrs != NULL) {
1041+
for (int i = 0; i < sym->descr.num_descrs; i++) {
1042+
if (sym->descr.descrs[i].slot_index == slot_index) {
1043+
old_value = PyJitRef_Wrap(allocation_base(ctx) + sym->descr.descrs[i].symbol);
1044+
break;
1045+
}
1046+
}
10501047
}
1051-
}
1052-
}
10531048

1054-
// Update the last modified timestamp
1055-
sym->descr.last_modified_index = curr_index;
1049+
// Check if have arena space allocated
1050+
if (sym->descr.descrs == NULL) {
1051+
sym->descr.descrs = descr_arena_alloc(ctx);
1052+
if (sym->descr.descrs == NULL) {
1053+
ctx->done = true;
1054+
ctx->out_of_space = true;
1055+
return _Py_uop_sym_new_null(ctx);
1056+
}
1057+
}
1058+
// Check if the slot already exists
1059+
for (int i = 0; i < sym->descr.num_descrs; i++) {
1060+
if (sym->descr.descrs[i].slot_index == slot_index) {
1061+
sym->descr.descrs[i].symbol = (uint16_t)(PyJitRef_Unwrap(value) - allocation_base(ctx));
1062+
assert(!_Py_uop_sym_is_null(old_value));
1063+
return old_value;
1064+
}
1065+
}
1066+
// Add new mapping if there's space
1067+
if (sym->descr.num_descrs < MAX_SYMBOLIC_DESCR_SIZE) {
1068+
int idx = sym->descr.num_descrs++;
1069+
sym->descr.descrs[idx].slot_index = slot_index;
1070+
sym->descr.descrs[idx].symbol = (uint16_t)(PyJitRef_Unwrap(value) - allocation_base(ctx));
1071+
}
10561072

1057-
// Check if have arena space allocated
1058-
if (sym->descr.descrs == NULL) {
1059-
sym->descr.descrs = descr_arena_alloc(ctx);
1060-
if (sym->descr.descrs == NULL) {
1061-
return PyJitRef_IsNull(old_value) ? _Py_uop_sym_new_not_null(ctx) : old_value;
1062-
}
1063-
}
1064-
// Check if the slot already exists
1065-
for (int i = 0; i < sym->descr.num_descrs; i++) {
1066-
if (sym->descr.descrs[i].slot_index == slot_index) {
1067-
sym->descr.descrs[i].symbol = (uint16_t)(PyJitRef_Unwrap(value) - allocation_base(ctx));
1068-
assert(!PyJitRef_IsNull(old_value));
1069-
return old_value;
1073+
// No value there. Objects are initialized to NULL, so it's safe.
1074+
return _Py_uop_sym_new_null(ctx);
10701075
}
1076+
default:
1077+
return _Py_uop_sym_new_unknown(ctx);
10711078
}
1072-
// Add new mapping if there's space
1073-
if (sym->descr.num_descrs < MAX_SYMBOLIC_DESCR_SIZE) {
1074-
int idx = sym->descr.num_descrs++;
1075-
sym->descr.descrs[idx].slot_index = slot_index;
1076-
sym->descr.descrs[idx].symbol = (uint16_t)(PyJitRef_Unwrap(value) - allocation_base(ctx));
1077-
}
1078-
1079-
return PyJitRef_IsNull(old_value) ? PyJitRef_Borrow(_Py_uop_sym_new_null(ctx)) : old_value;
10801079
}
10811080

10821081
// 0 on success, -1 on error.

‎Tools/cases_generator/analyzer.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -710,8 +710,14 @@ def has_error_without_pop(op: parser.CodeDef) -> bool:
710710
"_Py_set_eval_breaker_bit",
711711
"trigger_backoff_counter",
712712
"_PyThreadState_PopCStackRefSteal",
713+
"_PyFrame_PushTrampolineUnchecked",
713714
)
714715

716+
KNOWN_NON_ESCAPING_UOPS = {
717+
# This only calls escaping functions in the error path.
718+
"_CREATE_INIT_FRAME",
719+
}
720+
715721

716722
def check_escaping_calls(instr: parser.CodeDef, escapes: dict[SimpleStmt, EscapingCall]) -> None:
717723
error: lexer.Token | None = None
@@ -965,7 +971,7 @@ def compute_properties(op: parser.CodeDef) -> Properties:
965971
)
966972
error_with_pop = has_error_with_pop(op)
967973
error_without_pop = has_error_without_pop(op)
968-
escapes = stmt_escapes(op.block)
974+
escapes = not (isinstance(op, parser.InstDef) and op.name in KNOWN_NON_ESCAPING_UOPS) and stmt_escapes(op.block)
969975
pure = False if isinstance(op, parser.LabelDef) else "pure" in op.annotations
970976
no_save_ip = False if isinstance(op, parser.LabelDef) else "no_save_ip" in op.annotations
971977
unpredictable, branches_seen = stmt_has_jump_on_unpredictable_path(op.block, 0)

0 commit comments

Comments
 (0)