From d2f348594dce7d109ff2f2bacf45ff5d4d8e8189 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 30 Jun 2023 20:14:02 +0100 Subject: [PATCH 1/3] gh-106149: move unconditional jump direction resolution from optimizer to assembler --- Python/assemble.c | 34 ++++++++++++++++++++++++++++++++++ Python/flowgraph.c | 22 ++++++---------------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/Python/assemble.c b/Python/assemble.c index 5d566a381b3988..7db7708d98cca1 100644 --- a/Python/assemble.c +++ b/Python/assemble.c @@ -674,11 +674,45 @@ resolve_jump_offsets(instr_sequence *instrs) return SUCCESS; } +static int +resolve_unconditional_jumps(instr_sequence *instrs) +{ + /* Resolve directions of unconditional jumps */ + + for (int i = 0; i < instrs->s_used; i++) { + instruction *instr = &instrs->s_instrs[i]; + bool is_forward = (instr->i_oparg >= i); + switch(instr->i_opcode) { + case JUMP: + assert(SAME_OPCODE_METADATA(JUMP, JUMP_FORWARD)); + assert(SAME_OPCODE_METADATA(JUMP, JUMP_BACKWARD)); + instr->i_opcode = is_forward ? JUMP_FORWARD : JUMP_BACKWARD; + break; + case JUMP_NO_INTERRUPT: + assert(SAME_OPCODE_METADATA(JUMP_NO_INTERRUPT, JUMP_FORWARD)); + assert(SAME_OPCODE_METADATA(JUMP_NO_INTERRUPT, JUMP_BACKWARD_NO_INTERRUPT)); + instr->i_opcode = is_forward ? + JUMP_FORWARD : JUMP_BACKWARD_NO_INTERRUPT; + break; + default: + if (OPCODE_HAS_JUMP(instr->i_opcode) && + IS_PSEUDO_INSTR(instr->i_opcode)) { + Py_UNREACHABLE(); + } + } + } + return SUCCESS; +} + PyCodeObject * _PyAssemble_MakeCodeObject(_PyCompile_CodeUnitMetadata *umd, PyObject *const_cache, PyObject *consts, int maxdepth, instr_sequence *instrs, int nlocalsplus, int code_flags, PyObject *filename) { + + if (resolve_unconditional_jumps(instrs) < 0) { + return NULL; + } if (resolve_jump_offsets(instrs) < 0) { return NULL; } diff --git a/Python/flowgraph.c b/Python/flowgraph.c index 429109b74beab8..213c993bb863a3 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -393,24 +393,17 @@ no_redundant_jumps(cfg_builder *g) { static int normalize_jumps_in_block(cfg_builder *g, basicblock *b) { cfg_instr *last = _PyCfg_BasicblockLastInstr(b); - if (last == NULL || !is_jump(last)) { + if (last == NULL || !is_jump(last) || + IS_UNCONDITIONAL_JUMP_OPCODE(last->i_opcode)) { return SUCCESS; } assert(!IS_ASSEMBLER_OPCODE(last->i_opcode)); + bool is_forward = last->i_target->b_visited == 0; - switch(last->i_opcode) { - case JUMP: - assert(SAME_OPCODE_METADATA(JUMP, JUMP_FORWARD)); - assert(SAME_OPCODE_METADATA(JUMP, JUMP_BACKWARD)); - last->i_opcode = is_forward ? JUMP_FORWARD : JUMP_BACKWARD; - return SUCCESS; - case JUMP_NO_INTERRUPT: - assert(SAME_OPCODE_METADATA(JUMP_NO_INTERRUPT, JUMP_FORWARD)); - assert(SAME_OPCODE_METADATA(JUMP_NO_INTERRUPT, JUMP_BACKWARD_NO_INTERRUPT)); - last->i_opcode = is_forward ? - JUMP_FORWARD : JUMP_BACKWARD_NO_INTERRUPT; - return SUCCESS; + if (is_forward) { + return SUCCESS; } + int reversed_opcode = 0; switch(last->i_opcode) { case POP_JUMP_IF_NOT_NONE: @@ -426,9 +419,6 @@ normalize_jumps_in_block(cfg_builder *g, basicblock *b) { reversed_opcode = POP_JUMP_IF_FALSE; break; } - if (is_forward) { - return SUCCESS; - } /* transform 'conditional jump T' to * 'reversed_jump b_next' followed by 'jump_backwards T' */ From 334610d52ac60c03bec7931a54a2569789c6e3c0 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 30 Jun 2023 20:37:37 +0100 Subject: [PATCH 2/3] add () in macro --- Python/opcode_metadata.h | 4 ++-- Tools/cases_generator/generate_cases.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Python/opcode_metadata.h b/Python/opcode_metadata.h index 5c7de77deba2d6..6a42775e091208 100644 --- a/Python/opcode_metadata.h +++ b/Python/opcode_metadata.h @@ -4,7 +4,7 @@ // Do not edit! -#define IS_PSEUDO_INSTR(OP) \ +#define IS_PSEUDO_INSTR(OP) ( \ ((OP) == LOAD_CLOSURE) || \ ((OP) == STORE_FAST_MAYBE_NULL) || \ ((OP) == LOAD_SUPER_METHOD) || \ @@ -17,7 +17,7 @@ ((OP) == SETUP_CLEANUP) || \ ((OP) == SETUP_WITH) || \ ((OP) == POP_BLOCK) || \ - 0 + 0) #define EXIT_TRACE 300 #define SET_IP 301 diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index ff88b63d3bdd80..61762fbd9d42f3 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -1310,10 +1310,10 @@ def write_metadata(self) -> None: def write_pseudo_instrs(self) -> None: """Write the IS_PSEUDO_INSTR macro""" - self.out.emit("\n\n#define IS_PSEUDO_INSTR(OP) \\") + self.out.emit("\n\n#define IS_PSEUDO_INSTR(OP) ( \\") for op in self.pseudos: self.out.emit(f" ((OP) == {op}) || \\") - self.out.emit(f" 0") + self.out.emit(f" 0)") def write_uop_items(self, make_text: typing.Callable[[str, int], str]) -> None: """Write '#define XXX NNN' for each uop""" From 6452df32a112b6aa635d0b602a6b649d72edb253 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 30 Jun 2023 21:23:50 +0100 Subject: [PATCH 3/3] fix edge case --- Python/assemble.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/assemble.c b/Python/assemble.c index 7db7708d98cca1..ff7bca22286cd7 100644 --- a/Python/assemble.c +++ b/Python/assemble.c @@ -681,7 +681,7 @@ resolve_unconditional_jumps(instr_sequence *instrs) for (int i = 0; i < instrs->s_used; i++) { instruction *instr = &instrs->s_instrs[i]; - bool is_forward = (instr->i_oparg >= i); + bool is_forward = (instr->i_oparg > i); switch(instr->i_opcode) { case JUMP: assert(SAME_OPCODE_METADATA(JUMP, JUMP_FORWARD));