From 5e4c199295e1518ffa29dc5c2172f0782ff66584 Mon Sep 17 00:00:00 2001 From: iritkatriel Date: Tue, 29 Nov 2022 16:05:04 +0000 Subject: [PATCH 1/5] gh-99876: make optimizer maintain the invariant that oparg == 0 for an instruction that does not have an arg --- Python/compile.c | 57 +++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/Python/compile.c b/Python/compile.c index dd8596defb8efe..a3bc4e71ce8147 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -171,6 +171,17 @@ struct instr { struct basicblock_ *i_except; /* target block when exception is raised */ }; +/* One arg*/ +#define INSTR_SET_OP1(I, OP, ARG) \ + assert(HAS_ARG(OP) || ((ARG) == 0)); \ + (I)->i_opcode = (OP); \ + (I)->i_oparg = (ARG); + +/* No args*/ +#define INSTR_SET_OP0(I, OP) \ + assert(!HAS_ARG(OP)); \ + INSTR_SET_OP1((I), (OP), 0); + typedef struct exceptstack { struct basicblock_ *handlers[CO_MAXBLOCKS+1]; int depth; @@ -218,7 +229,8 @@ instr_size(struct instr *instruction) { int opcode = instruction->i_opcode; assert(!IS_PSEUDO_OPCODE(opcode)); - int oparg = HAS_ARG(opcode) ? instruction->i_oparg : 0; + int oparg = instruction->i_oparg; + assert(HAS_ARG(opcode) || oparg == 0); int extended_args = (0xFFFFFF < oparg) + (0xFFFF < oparg) + (0xFF < oparg); int caches = _PyOpcode_Caches[opcode]; return extended_args + 1 + caches; @@ -229,7 +241,8 @@ write_instr(_Py_CODEUNIT *codestr, struct instr *instruction, int ilen) { int opcode = instruction->i_opcode; assert(!IS_PSEUDO_OPCODE(opcode)); - int oparg = HAS_ARG(opcode) ? instruction->i_oparg : 0; + int oparg = instruction->i_oparg; + assert(HAS_ARG(opcode) || oparg == 0); int caches = _PyOpcode_Caches[opcode]; switch (ilen - caches) { case 4: @@ -7598,7 +7611,7 @@ convert_exception_handlers_to_nops(basicblock *entryblock) { for (int i = 0; i < b->b_iused; i++) { struct instr *instr = &b->b_instr[i]; if (is_block_push(instr) || instr->i_opcode == POP_BLOCK) { - instr->i_opcode = NOP; + INSTR_SET_OP0(instr, NOP); } } } @@ -8723,7 +8736,7 @@ remove_redundant_jumps(cfg_builder *g) { } if (last->i_target == b->b_next) { assert(b->b_next->b_iused); - last->i_opcode = NOP; + INSTR_SET_OP0(last, NOP); } } } @@ -8988,10 +9001,9 @@ fold_tuple_on_constants(PyObject *const_cache, } Py_DECREF(newconst); for (int i = 0; i < n; i++) { - inst[i].i_opcode = NOP; + INSTR_SET_OP0(&inst[i], NOP); } - inst[n].i_opcode = LOAD_CONST; - inst[n].i_oparg = (int)index; + INSTR_SET_OP1(&inst[n], LOAD_CONST, (int)index); return 0; } @@ -9088,7 +9100,7 @@ swaptimize(basicblock *block, int *ix) } // NOP out any unused instructions: while (0 <= current) { - instructions[current--].i_opcode = NOP; + INSTR_SET_OP0(&instructions[current--], NOP); } PyMem_Free(stack); *ix += len - 1; @@ -9154,7 +9166,7 @@ apply_static_swaps(basicblock *block, int i) } } // Success! - swap->i_opcode = NOP; + INSTR_SET_OP0(swap, NOP); struct instr temp = block->b_instr[j]; block->b_instr[j] = block->b_instr[k]; block->b_instr[k] = temp; @@ -9191,7 +9203,7 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) assert(PyDict_CheckExact(const_cache)); assert(PyList_CheckExact(consts)); struct instr nop; - nop.i_opcode = NOP; + INSTR_SET_OP0(&nop, NOP); struct instr *target; for (int i = 0; i < bb->b_iused; i++) { struct instr *inst = &bb->b_instr[i]; @@ -9225,13 +9237,13 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) if (is_true == -1) { goto error; } - inst->i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); jump_if_true = nextop == POP_JUMP_IF_TRUE; if (is_true == jump_if_true) { bb->b_instr[i+1].i_opcode = JUMP; } else { - bb->b_instr[i+1].i_opcode = NOP; + INSTR_SET_OP0(&bb->b_instr[i + 1], NOP); } break; case JUMP_IF_FALSE_OR_POP: @@ -9250,8 +9262,8 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) bb->b_instr[i+1].i_opcode = JUMP; } else { - inst->i_opcode = NOP; - bb->b_instr[i+1].i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); + INSTR_SET_OP0(&bb->b_instr[i + 1], NOP); } break; case IS_OP: @@ -9262,8 +9274,8 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) int jump_op = i+2 < bb->b_iused ? bb->b_instr[i+2].i_opcode : 0; if (Py_IsNone(cnt) && (jump_op == POP_JUMP_IF_FALSE || jump_op == POP_JUMP_IF_TRUE)) { unsigned char nextarg = bb->b_instr[i+1].i_oparg; - inst->i_opcode = NOP; - bb->b_instr[i+1].i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); + INSTR_SET_OP0(&bb->b_instr[i + 1], NOP); bb->b_instr[i+2].i_opcode = nextarg ^ (jump_op == POP_JUMP_IF_FALSE) ? POP_JUMP_IF_NOT_NONE : POP_JUMP_IF_NONE; } @@ -9281,12 +9293,12 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) if (nextop == UNPACK_SEQUENCE && oparg == bb->b_instr[i+1].i_oparg) { switch(oparg) { case 1: - inst->i_opcode = NOP; - bb->b_instr[i+1].i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); + INSTR_SET_OP0(&bb->b_instr[i + 1], NOP); continue; case 2: case 3: - inst->i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); bb->b_instr[i+1].i_opcode = SWAP; continue; } @@ -9395,7 +9407,7 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) break; case SWAP: if (oparg == 1) { - inst->i_opcode = NOP; + INSTR_SET_OP0(inst, NOP); break; } if (swaptimize(bb, &i)) { @@ -9407,8 +9419,7 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) break; case PUSH_NULL: if (nextop == LOAD_GLOBAL && (inst[1].i_opcode & 1) == 0) { - inst->i_opcode = NOP; - inst->i_oparg = 0; + INSTR_SET_OP0(inst, NOP); inst[1].i_oparg |= 1; } break; @@ -9437,7 +9448,7 @@ inline_small_exit_blocks(basicblock *bb) { } basicblock *target = last->i_target; if (basicblock_exits_scope(target) && target->b_iused <= MAX_COPY_SIZE) { - last->i_opcode = NOP; + INSTR_SET_OP0(last, NOP); if (basicblock_append_instructions(bb, target) < 0) { return -1; } From a9bfc0d75d72ad921ce95ce48b63abfbe7fe2e0d Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Tue, 29 Nov 2022 17:01:29 +0000 Subject: [PATCH 2/5] magic number --- Lib/importlib/_bootstrap_external.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 71a16064b8ec0a..1b5ec9e5df3f4e 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -426,6 +426,7 @@ def _write_atomic(path, data, mode=0o666): # Python 3.12a1 3510 (FOR_ITER leaves iterator on the stack) # Python 3.12a1 3511 (Add STOPITERATION_ERROR instruction) # Python 3.12a1 3512 (Remove all unused consts from code objects) +# Python 3.12a1 3513 (Optimizer ensures oparg is 0 when !HAS_ARG) # Python 3.13 will start with 3550 @@ -438,7 +439,7 @@ def _write_atomic(path, data, mode=0o666): # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array # in PC/launcher.c must also be updated. -MAGIC_NUMBER = (3512).to_bytes(2, 'little') + b'\r\n' +MAGIC_NUMBER = (3513).to_bytes(2, 'little') + b'\r\n' _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c From dfff7b8842b922b2a66ffc49fcc0ab41574d99fd Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Tue, 29 Nov 2022 18:36:29 +0000 Subject: [PATCH 3/5] Revert "magic number" This reverts commit a9bfc0d75d72ad921ce95ce48b63abfbe7fe2e0d. --- Lib/importlib/_bootstrap_external.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 1b5ec9e5df3f4e..71a16064b8ec0a 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -426,7 +426,6 @@ def _write_atomic(path, data, mode=0o666): # Python 3.12a1 3510 (FOR_ITER leaves iterator on the stack) # Python 3.12a1 3511 (Add STOPITERATION_ERROR instruction) # Python 3.12a1 3512 (Remove all unused consts from code objects) -# Python 3.12a1 3513 (Optimizer ensures oparg is 0 when !HAS_ARG) # Python 3.13 will start with 3550 @@ -439,7 +438,7 @@ def _write_atomic(path, data, mode=0o666): # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array # in PC/launcher.c must also be updated. -MAGIC_NUMBER = (3513).to_bytes(2, 'little') + b'\r\n' +MAGIC_NUMBER = (3512).to_bytes(2, 'little') + b'\r\n' _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c From e9110013a5aec1a794f14caa56b93dda44839c3d Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Tue, 29 Nov 2022 18:41:10 +0000 Subject: [PATCH 4/5] fix INSTR_SET_OP1 macro to access arg once --- Python/compile.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Python/compile.c b/Python/compile.c index a3bc4e71ce8147..dbf7b8c47b49c4 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -173,9 +173,12 @@ struct instr { /* One arg*/ #define INSTR_SET_OP1(I, OP, ARG) \ - assert(HAS_ARG(OP) || ((ARG) == 0)); \ - (I)->i_opcode = (OP); \ - (I)->i_oparg = (ARG); + do { \ + struct instr *__instr__ptr__ = (I); \ + assert(HAS_ARG(OP) || ((ARG) == 0)); \ + (__instr__ptr__)->i_opcode = (OP); \ + (__instr__ptr__)->i_oparg = (ARG); \ + } while (0); /* No args*/ #define INSTR_SET_OP0(I, OP) \ From 857db4e34996231f876add2704669af43bbd5ac0 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 30 Nov 2022 12:50:31 +0000 Subject: [PATCH 5/5] implement Mark's suggestions --- Python/compile.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Python/compile.c b/Python/compile.c index dbf7b8c47b49c4..54328b8e044b15 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -174,16 +174,20 @@ struct instr { /* One arg*/ #define INSTR_SET_OP1(I, OP, ARG) \ do { \ - struct instr *__instr__ptr__ = (I); \ - assert(HAS_ARG(OP) || ((ARG) == 0)); \ - (__instr__ptr__)->i_opcode = (OP); \ - (__instr__ptr__)->i_oparg = (ARG); \ + assert(HAS_ARG(OP)); \ + struct instr *_instr__ptr_ = (I); \ + _instr__ptr_->i_opcode = (OP); \ + _instr__ptr_->i_oparg = (ARG); \ } while (0); /* No args*/ #define INSTR_SET_OP0(I, OP) \ - assert(!HAS_ARG(OP)); \ - INSTR_SET_OP1((I), (OP), 0); + do { \ + assert(!HAS_ARG(OP)); \ + struct instr *_instr__ptr_ = (I); \ + _instr__ptr_->i_opcode = (OP); \ + _instr__ptr_->i_oparg = 0; \ + } while (0); typedef struct exceptstack { struct basicblock_ *handlers[CO_MAXBLOCKS+1];