From af97a7e20004c2de27eeeacd7f1a368940156deb Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 25 Jan 2023 12:58:34 -0800 Subject: [PATCH 01/13] Add parser support for conditional stack effects (Semantics aren't there yet, and the test fails.) --- Tools/cases_generator/parser.py | 49 ++++++++++++++----------- Tools/cases_generator/test_generator.py | 36 +++++++++++++----- 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/Tools/cases_generator/parser.py b/Tools/cases_generator/parser.py index c2cebe96ccd6be..ced66faee4931f 100644 --- a/Tools/cases_generator/parser.py +++ b/Tools/cases_generator/parser.py @@ -48,9 +48,6 @@ def to_text(self, dedent: int = 0) -> str: context = self.context if not context: return "" - tokens = context.owner.tokens - begin = context.begin - end = context.end return lx.to_text(self.tokens, dedent) @property @@ -74,13 +71,13 @@ class Block(Node): class StackEffect(Node): name: str type: str = "" # Optional `:type` + cond: str = "" # Optional `if (cond)` size: str = "" # Optional `[size]` - # Note: we can have type or size but not both - # TODO: condition (can be combined with type but not size) + # Note: size cannot be combined with type or cond @dataclass -class Dimension(Node): +class Expression(Node): size: str @@ -239,31 +236,39 @@ def cache_effect(self) -> CacheEffect | None: @contextual def stack_effect(self) -> StackEffect | None: - # IDENTIFIER - # | IDENTIFIER ':' IDENTIFIER - # | IDENTIFIER '[' dimension ']' - # TODO: Conditions + # IDENTIFIER [':' IDENTIFIER] ['if' '(' expression ')'] + # | IDENTIFIER '[' expression ']' if tkn := self.expect(lx.IDENTIFIER): + type_text = "" if self.expect(lx.COLON): - typ = self.require(lx.IDENTIFIER) - return StackEffect(tkn.text, typ.text) - elif self.expect(lx.LBRACKET): - if not (dim := self.dimension()): - raise self.make_syntax_error("Expected dimension") + type_text = self.require(lx.IDENTIFIER).text.strip() + cond_text = "" + if self.expect(lx.IF): + self.require(lx.LPAREN) + if not (cond := self.expression()): + raise self.make_syntax_error("Expected condition") + self.require(lx.RPAREN) + cond_text = cond.text.strip() + size_text = "" + if self.expect(lx.LBRACKET): + if type_text or cond_text: + raise self.make_syntax_error("Unexpected [") + if not (size := self.expression()): + raise self.make_syntax_error("Expected expression") self.require(lx.RBRACKET) - return StackEffect(tkn.text, "PyObject **", dim.text.strip()) - else: - return StackEffect(tkn.text) + type_text = "PyObject **" + size_text = size.text.strip() + return StackEffect(tkn.text, type_text, cond_text, size_text) @contextual - def dimension(self) -> Dimension | None: + def expression(self) -> Expression | None: tokens: list[lx.Token] = [] - while (tkn := self.peek()) and tkn.kind != lx.RBRACKET: + while (tkn := self.peek()) and tkn.kind not in (lx.RBRACKET, lx.RPAREN): tokens.append(tkn) self.next() if not tokens: return None - return Dimension(lx.to_text(tokens).strip()) + return Expression(lx.to_text(tokens).strip()) @contextual def super_def(self) -> Super | None: @@ -366,7 +371,7 @@ def members(self) -> list[str] | None: return None @contextual - def block(self) -> Block: + def block(self) -> Block | None: if self.c_blob(): return Block() diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index cf58e6aaf2b37c..8eb10010c2b4f6 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -9,19 +9,19 @@ def test_effect_sizes(): input_effects = [ - x := StackEffect("x", "", ""), - y := StackEffect("y", "", "oparg"), - z := StackEffect("z", "", "oparg*2"), + x := StackEffect("x", "", "", ""), + y := StackEffect("y", "", "", "oparg"), + z := StackEffect("z", "", "", "oparg*2"), ] output_effects = [ - a := StackEffect("a", "", ""), - b := StackEffect("b", "", "oparg*4"), - c := StackEffect("c", "", ""), + StackEffect("a", "", "", ""), + StackEffect("b", "", "", "oparg*4"), + StackEffect("c", "", "", ""), ] other_effects = [ - p := StackEffect("p", "", "oparg<<1"), - q := StackEffect("q", "", ""), - r := StackEffect("r", "", ""), + StackEffect("p", "", "", "oparg<<1"), + StackEffect("q", "", "", ""), + StackEffect("r", "", "", ""), ] assert generate_cases.effect_size(x) == (1, "") assert generate_cases.effect_size(y) == (0, "oparg") @@ -476,3 +476,21 @@ def test_register(): } """ run_cases_test(input, output) + +def test_cond_effect(): + input = """ + inst(OP, (input if (oparg & 1) -- output if (oparg & 2))) { + output = spam(oparg, input); + } + """ + output = """ + TARGET(OP) { + PyObject *input = NULL; + if (oparg & 1) { input = POP(); } + PyObject *output; + output = spam(oparg, input); + if (oparg & 2) { PUSH(output); } + DISPATCH(); + } + """ + run_cases_test(input, output) From fbcdde371e1636927b542aabd1293b7e071d1dc6 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 25 Jan 2023 13:08:04 -0800 Subject: [PATCH 02/13] Fix a typo in test_generator.py that made it into main :-( --- Tools/cases_generator/test_generator.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index 8eb10010c2b4f6..1d2d71d68c5472 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -358,8 +358,7 @@ def test_macro_instruction(): { PyObject *arg1 = _tmp_1; PyObject *interim; - uint16_t counter = re - ad_u16(&next_instr[0].cache); + uint16_t counter = read_u16(&next_instr[0].cache); interim = op1(arg1); _tmp_1 = interim; } From 27246e9d2b2c10d7e72677ca6c7ffd83cd4136a7 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 25 Jan 2023 15:07:27 -0800 Subject: [PATCH 03/13] Make conditional effects work, in theory --- Python/generated_cases.c.h | 2 +- Tools/cases_generator/generate_cases.py | 50 ++++++++++++++++++++++--- Tools/cases_generator/parser.py | 2 + Tools/cases_generator/test_generator.py | 17 ++++++--- 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 287a1f1f042089..76eb0a3d0415be 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2273,8 +2273,8 @@ if (!Py_IsNone(match)) { PyErr_SetExcInfo(NULL, Py_NewRef(match), NULL); } - POKE(1, match); POKE(2, rest); + POKE(1, match); DISPATCH(); } diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index b7942410c82fc3..01615fcf9e9d0d 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -163,6 +163,11 @@ def assign(self, dst: StackEffect, src: StackEffect): cast = self.cast(dst, src) if m := re.match(r"^PEEK\((.*)\)$", dst.name): self.emit(f"POKE({m.group(1)}, {cast}{src.name});") + elif m := re.match(r"^POP\(\)$", dst.name): + if src.cond: + self.emit(f"if ({src.cond}) {{ PUSH({cast}{src.name}); }}") + else: + self.emit(f"PUSH({cast}{src.name});") elif m := re.match(r"^&PEEK\(.*\)$", dst.name): # NOTE: MOVE_ITEMS() does not actually exist. # The only supported output array forms are: @@ -220,10 +225,23 @@ def __init__(self, inst: parser.InstDef): effect for effect in inst.inputs if isinstance(effect, parser.CacheEffect) ] self.cache_offset = sum(c.size for c in self.cache_effects) + self.input_effects = [ effect for effect in inst.inputs if isinstance(effect, StackEffect) ] + manual = False + for effect in self.input_effects: + if effect.cond: + manual = True + effect.manual = manual + self.output_effects = inst.outputs # For consistency/completeness + manual = False + for effect in self.output_effects: + if effect.cond: + manual = True + effect.manual = manual + unmoved_names: set[str] = set() for ieffect, oeffect in zip(self.input_effects, self.output_effects): if ieffect.name == oeffect.name: @@ -276,15 +294,26 @@ def write(self, out: Formatter) -> None: # Write input stack effect variable declarations and initializations ieffects = list(reversed(self.input_effects)) for i, ieffect in enumerate(ieffects): - isize = string_effect_size(list_effect_size(ieffects[:i+1])) + isize = string_effect_size(list_effect_size([ieff for ieff in ieffects[:i+1] if not ieff.manual])) if ieffect.size: + assert not ieffect.manual, "Manual array effects not yet supported" src = StackEffect(f"&PEEK({isize})", "PyObject **") else: - src = StackEffect(f"PEEK({isize})", "") + if ieffect.manual: + if ieffect.cond: + src = StackEffect(f"({ieffect.cond}) ? POP() : NULL", "") + else: + src = StackEffect(f"POP()", "") + else: + src = StackEffect(f"PEEK({isize})", "") out.declare(ieffect, src) + # if ieffect.cond: + # assert ieffect.manual, "Conditional effects must be manual" + # out.emit(f"if ({ieffect.cond}) {{ {ieffect.name} = POP(); }}") else: # Write input register variable declarations and initializations for ieffect, reg in zip(self.input_effects, self.input_registers): + assert not ieffect.manual, "Manual register effects not yet supported" src = StackEffect(reg, "") out.declare(ieffect, src) @@ -304,22 +333,33 @@ def write(self, out: Formatter) -> None: if not self.register: # Write net stack growth/shrinkage - out.stack_adjust(0, self.input_effects, self.output_effects) + out.stack_adjust(0, + [ieff for ieff in self.input_effects if not ieff.manual], + [oeff for oeff in self.output_effects if not oeff.manual]) # Write output stack effect assignments oeffects = list(reversed(self.output_effects)) + real_oeffects: list[tuple[StackEffect, StackEffect]] = [] for i, oeffect in enumerate(oeffects): if oeffect.name in self.unmoved_names: continue - osize = string_effect_size(list_effect_size(oeffects[:i+1])) + osize = string_effect_size(list_effect_size([oeff for oeff in oeffects[:i+1] if not oeff.manual])) if oeffect.size: + assert not oeffect.manual, "Manual array effects not yet supported" dst = StackEffect(f"&PEEK({osize})", "PyObject **") else: - dst = StackEffect(f"PEEK({osize})", "") + if oeffect.manual: + dst = StackEffect(f"POP()", "") + else: + dst = StackEffect(f"PEEK({osize})", "") + real_oeffects.append((dst, oeffect)) + for dst, oeffect in reversed(real_oeffects): out.assign(dst, oeffect) else: # Write output register assignments for oeffect, reg in zip(self.output_effects, self.output_registers): + assert not oeffect.manual, "Manual register effects not yet supported" + src = StackEffect(reg, "") dst = StackEffect(reg, "") out.assign(dst, oeffect) diff --git a/Tools/cases_generator/parser.py b/Tools/cases_generator/parser.py index ced66faee4931f..1c0dd8b6ef9f58 100644 --- a/Tools/cases_generator/parser.py +++ b/Tools/cases_generator/parser.py @@ -74,6 +74,8 @@ class StackEffect(Node): cond: str = "" # Optional `if (cond)` size: str = "" # Optional `[size]` # Note: size cannot be combined with type or cond + # 'manual' is used to decide between PEEK() and POP() + manual: bool = field(init=False, compare=False, default=False) @dataclass diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index 1d2d71d68c5472..224d00bbfb2cd2 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -411,9 +411,9 @@ def test_array_output(): spam(); STACK_GROW(2); STACK_GROW(oparg*3); - POKE(1, above); - MOVE_ITEMS(&PEEK(1 + oparg*3), values, oparg*3); POKE(2 + oparg*3, below); + MOVE_ITEMS(&PEEK(1 + oparg*3), values, oparg*3); + POKE(1, above); DISPATCH(); } """ @@ -431,8 +431,8 @@ def test_array_input_output(): PyObject *below = PEEK(1 + oparg); PyObject *above; spam(); - POKE(1, above); MOVE_ITEMS(&PEEK(1 + oparg), values, oparg); + POKE(1, above); DISPATCH(); } """ @@ -478,17 +478,22 @@ def test_register(): def test_cond_effect(): input = """ - inst(OP, (input if (oparg & 1) -- output if (oparg & 2))) { + inst(OP, (aa, input if (oparg & 1), cc -- xx, output if (oparg & 2), zz)) { output = spam(oparg, input); } """ output = """ TARGET(OP) { - PyObject *input = NULL; - if (oparg & 1) { input = POP(); } + PyObject *cc = POP(); + PyObject *input = (oparg & 1) ? POP() : NULL; + PyObject *aa = PEEK(1); + PyObject *xx; PyObject *output; + PyObject *zz; output = spam(oparg, input); + POKE(1, xx); if (oparg & 2) { PUSH(output); } + PUSH(zz); DISPATCH(); } """ From 667630e6ca0dc6aef7719b82079a1d11d1642801 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 25 Jan 2023 16:24:31 -0800 Subject: [PATCH 04/13] Convert LOAD_ATTR to new format First use of conditional stack effects. --- Python/bytecodes.c | 38 +++++++++++++------------------------ Python/generated_cases.c.h | 39 +++++++++++++++++--------------------- Python/opcode_metadata.h | 8 ++++---- 3 files changed, 34 insertions(+), 51 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index e5769f61fc28d0..c7b8e233443374 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -51,7 +51,7 @@ // Dummy variables for stack effects. static PyObject *value, *value1, *value2, *left, *right, *res, *sum, *prod, *sub; -static PyObject *container, *start, *stop, *v, *lhs, *rhs; +static PyObject *container, *start, *stop, *v, *lhs, *rhs, *res2; static PyObject *list, *tuple, *dict, *owner, *set, *str, *tup, *map, *keys; static PyObject *exit_func, *lasti, *val, *retval, *obj, *iter; static PyObject *aiter, *awaitable, *iterable, *w, *exc_value, *bc; @@ -1438,13 +1438,11 @@ dummy_func( PREDICT(JUMP_BACKWARD); } - // error: LOAD_ATTR has irregular stack effect - inst(LOAD_ATTR) { + inst(LOAD_ATTR, (unused/9, owner -- res, res2 if (oparg & 1))) { #if ENABLE_SPECIALIZATION _PyAttrCache *cache = (_PyAttrCache *)next_instr; if (ADAPTIVE_COUNTER_IS_ZERO(cache->counter)) { assert(cframe.use_tracing == 0); - PyObject *owner = TOP(); PyObject *name = GETITEM(names, oparg>>1); next_instr--; _Py_Specialize_LoadAttr(owner, next_instr, name); @@ -1454,26 +1452,18 @@ dummy_func( DECREMENT_ADAPTIVE_COUNTER(cache->counter); #endif /* ENABLE_SPECIALIZATION */ PyObject *name = GETITEM(names, oparg >> 1); - PyObject *owner = TOP(); if (oparg & 1) { - /* Designed to work in tandem with CALL. */ + /* Designed to work in tandem with CALL, pushes two values. */ PyObject* meth = NULL; - - int meth_found = _PyObject_GetMethod(owner, name, &meth); - - if (meth == NULL) { - /* Most likely attribute wasn't found. */ - goto error; - } - - if (meth_found) { + if (_PyObject_GetMethod(owner, name, &meth)) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ - SET_TOP(meth); - PUSH(owner); // self + assert(meth != NULL); // No errors on this branch + res = meth; + res2 = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -1483,20 +1473,18 @@ dummy_func( NULL | meth | arg1 | ... | argN */ - SET_TOP(NULL); Py_DECREF(owner); - PUSH(meth); + ERROR_IF(meth == NULL, error); + res = NULL; + res2 = meth; } } else { - PyObject *res = PyObject_GetAttr(owner, name); - if (res == NULL) { - goto error; - } + /* Classic, pushes one value. */ + res = PyObject_GetAttr(owner, name); Py_DECREF(owner); - SET_TOP(res); + ERROR_IF(res == NULL, error); } - JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR); } // error: LOAD_ATTR has irregular stack effect diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 76eb0a3d0415be..7278f8a351cbb0 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1745,11 +1745,13 @@ TARGET(LOAD_ATTR) { PREDICTED(LOAD_ATTR); + PyObject *owner = PEEK(1); + PyObject *res; + PyObject *res2; #if ENABLE_SPECIALIZATION _PyAttrCache *cache = (_PyAttrCache *)next_instr; if (ADAPTIVE_COUNTER_IS_ZERO(cache->counter)) { assert(cframe.use_tracing == 0); - PyObject *owner = TOP(); PyObject *name = GETITEM(names, oparg>>1); next_instr--; _Py_Specialize_LoadAttr(owner, next_instr, name); @@ -1759,26 +1761,18 @@ DECREMENT_ADAPTIVE_COUNTER(cache->counter); #endif /* ENABLE_SPECIALIZATION */ PyObject *name = GETITEM(names, oparg >> 1); - PyObject *owner = TOP(); if (oparg & 1) { - /* Designed to work in tandem with CALL. */ + /* Designed to work in tandem with CALL, pushes two values. */ PyObject* meth = NULL; - - int meth_found = _PyObject_GetMethod(owner, name, &meth); - - if (meth == NULL) { - /* Most likely attribute wasn't found. */ - goto error; - } - - if (meth_found) { + if (_PyObject_GetMethod(owner, name, &meth)) { /* We can bypass temporary bound method object. meth is unbound method and obj is self. meth | self | arg1 | ... | argN */ - SET_TOP(meth); - PUSH(owner); // self + assert(meth != NULL); // No errors on this branch + res = meth; + res2 = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -1788,20 +1782,21 @@ NULL | meth | arg1 | ... | argN */ - SET_TOP(NULL); Py_DECREF(owner); - PUSH(meth); + if (meth == NULL) goto pop_1_error; + res = NULL; + res2 = meth; } } else { - PyObject *res = PyObject_GetAttr(owner, name); - if (res == NULL) { - goto error; - } + /* Classic, pushes one value. */ + res = PyObject_GetAttr(owner, name); Py_DECREF(owner); - SET_TOP(res); + if (res == NULL) goto pop_1_error; } - JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR); + POKE(1, res); + if (oparg & 1) { PUSH(res2); } + JUMPBY(9); DISPATCH(); } diff --git a/Python/opcode_metadata.h b/Python/opcode_metadata.h index cca86629e48d16..2de71077da767d 100644 --- a/Python/opcode_metadata.h +++ b/Python/opcode_metadata.h @@ -185,7 +185,7 @@ _PyOpcode_num_popped(int opcode, int oparg) { case MAP_ADD: return 2; case LOAD_ATTR: - return -1; + return 1; case LOAD_ATTR_INSTANCE_VALUE: return -1; case LOAD_ATTR_MODULE: @@ -531,7 +531,7 @@ _PyOpcode_num_pushed(int opcode, int oparg) { case MAP_ADD: return 0; case LOAD_ATTR: - return -1; + return 2; case LOAD_ATTR_INSTANCE_VALUE: return -1; case LOAD_ATTR_MODULE: @@ -694,7 +694,7 @@ _PyOpcode_num_pushed(int opcode, int oparg) { } #endif enum Direction { DIR_NONE, DIR_READ, DIR_WRITE }; -enum InstructionFormat { INSTR_FMT_IB, INSTR_FMT_IBC, INSTR_FMT_IBC0, INSTR_FMT_IBC000, INSTR_FMT_IBIB, INSTR_FMT_IX, INSTR_FMT_IXC, INSTR_FMT_IXC000 }; +enum InstructionFormat { INSTR_FMT_IB, INSTR_FMT_IBC, INSTR_FMT_IBC0, INSTR_FMT_IBC000, INSTR_FMT_IBC00000000, INSTR_FMT_IBIB, INSTR_FMT_IX, INSTR_FMT_IXC, INSTR_FMT_IXC000 }; struct opcode_metadata { enum Direction dir_op1; enum Direction dir_op2; @@ -791,7 +791,7 @@ struct opcode_metadata { [DICT_UPDATE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, [DICT_MERGE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, [MAP_ADD] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, - [LOAD_ATTR] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, + [LOAD_ATTR] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IBC00000000 }, [LOAD_ATTR_INSTANCE_VALUE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, [LOAD_ATTR_MODULE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, [LOAD_ATTR_WITH_HINT] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB }, From 69dbc5e87f8edb9375b500140fdef749dcf779f4 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 25 Jan 2023 16:38:01 -0800 Subject: [PATCH 05/13] Conditionalize effect_size() --- Python/opcode_metadata.h | 2 +- Tools/cases_generator/generate_cases.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Python/opcode_metadata.h b/Python/opcode_metadata.h index 2de71077da767d..568a0b0844016e 100644 --- a/Python/opcode_metadata.h +++ b/Python/opcode_metadata.h @@ -531,7 +531,7 @@ _PyOpcode_num_pushed(int opcode, int oparg) { case MAP_ADD: return 0; case LOAD_ATTR: - return 2; + return ((oparg & 1) != 0) + 1; case LOAD_ATTR_INSTANCE_VALUE: return -1; case LOAD_ATTR_MODULE: diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 01615fcf9e9d0d..ade7bf34625f37 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -59,7 +59,10 @@ def effect_size(effect: StackEffect) -> tuple[int, str]: At most one of these will be non-zero / non-empty. """ if effect.size: + assert not effect.cond, "Manual effects should be conditional" return 0, effect.size + elif effect.cond: + return 0, f"{maybe_parenthesize(effect.cond)} != 0" else: return 1, "" @@ -778,10 +781,10 @@ def get_stack_effect_info( self, thing: parser.InstDef | parser.Super | parser.Macro ) -> tuple[Instruction, str, str]: - def effect_str(effect: list[StackEffect]) -> str: + def effect_str(effects: list[StackEffect]) -> str: if getattr(thing, 'kind', None) == 'legacy': return str(-1) - n_effect, sym_effect = list_effect_size(effect) + n_effect, sym_effect = list_effect_size(effects) if sym_effect: return f"{sym_effect} + {n_effect}" if n_effect else sym_effect return str(n_effect) From 6a03438b861071e327d558862123c640d6745208 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 26 Jan 2023 08:45:17 -0800 Subject: [PATCH 06/13] Fix two bugs in case generator - UndefinedLocalError when generating metadata for an 'op' - Accidental newline inserted in test_generator.py --- Tools/cases_generator/generate_cases.py | 10 +++++++--- Tools/cases_generator/test_generator.py | 3 +-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index b7942410c82fc3..9d894d2ff57455 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -736,7 +736,7 @@ def stack_analysis( def get_stack_effect_info( self, thing: parser.InstDef | parser.Super | parser.Macro - ) -> tuple[Instruction, str, str]: + ) -> tuple[Instruction|None, str, str]: def effect_str(effect: list[StackEffect]) -> str: if getattr(thing, 'kind', None) == 'legacy': @@ -752,6 +752,9 @@ def effect_str(effect: list[StackEffect]) -> str: instr = self.instrs[thing.name] popped = effect_str(instr.input_effects) pushed = effect_str(instr.output_effects) + else: + instr = None + popped = pushed = "", "" case parser.Super(): instr = self.super_instrs[thing.name] popped = '+'.join(effect_str(comp.instr.input_effects) for comp in instr.parts) @@ -770,8 +773,9 @@ def write_stack_effect_functions(self) -> None: pushed_data = [] for thing in self.everything: instr, popped, pushed = self.get_stack_effect_info(thing) - popped_data.append( (instr, popped) ) - pushed_data.append( (instr, pushed) ) + if instr is not None: + popped_data.append( (instr, popped) ) + pushed_data.append( (instr, pushed) ) def write_function(direction: str, data: list[tuple[Instruction, str]]) -> None: self.out.emit("\n#ifndef NDEBUG"); diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index cf58e6aaf2b37c..bd1b974399abdd 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -358,8 +358,7 @@ def test_macro_instruction(): { PyObject *arg1 = _tmp_1; PyObject *interim; - uint16_t counter = re - ad_u16(&next_instr[0].cache); + uint16_t counter = read_u16(&next_instr[0].cache); interim = op1(arg1); _tmp_1 = interim; } From a0fa513d384add21868a54cbc8335b7a3996f28c Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 26 Jan 2023 08:53:08 -0800 Subject: [PATCH 07/13] Blackify --- Tools/cases_generator/generate_cases.py | 109 ++++++++++++++++-------- 1 file changed, 75 insertions(+), 34 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index edb788eed5019a..c208b65deeb5e3 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -26,7 +26,9 @@ ) BEGIN_MARKER = "// BEGIN BYTECODES //" END_MARKER = "// END BYTECODES //" -RE_PREDICTED = r"^\s*(?:PREDICT\(|GO_TO_INSTRUCTION\(|DEOPT_IF\(.*?,\s*)(\w+)\);\s*(?://.*)?$" +RE_PREDICTED = ( + r"^\s*(?:PREDICT\(|GO_TO_INSTRUCTION\(|DEOPT_IF\(.*?,\s*)(\w+)\);\s*(?://.*)?$" +) UNUSED = "unused" BITS_PER_CODE_UNIT = 16 @@ -135,7 +137,12 @@ def block(self, head: str): yield self.emit("}") - def stack_adjust(self, diff: int, input_effects: list[StackEffect], output_effects: list[StackEffect]): + def stack_adjust( + self, + diff: int, + input_effects: list[StackEffect], + output_effects: list[StackEffect], + ): # TODO: Get rid of 'diff' parameter shrink, isym = list_effect_size(input_effects) grow, osym = list_effect_size(output_effects) @@ -255,7 +262,7 @@ def __init__(self, inst: parser.InstDef): if self.register: num_regs = len(self.input_effects) + len(self.output_effects) num_dummies = (num_regs // 2) * 2 + 1 - num_regs - fmt = "I" + "B"*num_regs + "X"*num_dummies + fmt = "I" + "B" * num_regs + "X" * num_dummies else: if variable_used(inst, "oparg"): fmt = "IB" @@ -297,18 +304,22 @@ def write(self, out: Formatter) -> None: # Write input stack effect variable declarations and initializations ieffects = list(reversed(self.input_effects)) for i, ieffect in enumerate(ieffects): - isize = string_effect_size(list_effect_size([ieff for ieff in ieffects[:i+1] if not ieff.manual])) + isize = string_effect_size( + list_effect_size( + [ieff for ieff in ieffects[: i + 1] if not ieff.manual] + ) + ) if ieffect.size: assert not ieffect.manual, "Manual array effects not yet supported" src = StackEffect(f"&PEEK({isize})", "PyObject **") else: if ieffect.manual: if ieffect.cond: - src = StackEffect(f"({ieffect.cond}) ? POP() : NULL", "") + src = StackEffect(f"({ieffect.cond}) ? POP() : NULL", "") else: - src = StackEffect(f"POP()", "") + src = StackEffect(f"POP()", "") else: - src = StackEffect(f"PEEK({isize})", "") + src = StackEffect(f"PEEK({isize})", "") out.declare(ieffect, src) # if ieffect.cond: # assert ieffect.manual, "Conditional effects must be manual" @@ -336,9 +347,11 @@ def write(self, out: Formatter) -> None: if not self.register: # Write net stack growth/shrinkage - out.stack_adjust(0, + out.stack_adjust( + 0, [ieff for ieff in self.input_effects if not ieff.manual], - [oeff for oeff in self.output_effects if not oeff.manual]) + [oeff for oeff in self.output_effects if not oeff.manual], + ) # Write output stack effect assignments oeffects = list(reversed(self.output_effects)) @@ -346,7 +359,11 @@ def write(self, out: Formatter) -> None: for i, oeffect in enumerate(oeffects): if oeffect.name in self.unmoved_names: continue - osize = string_effect_size(list_effect_size([oeff for oeff in oeffects[:i+1] if not oeff.manual])) + osize = string_effect_size( + list_effect_size( + [oeff for oeff in oeffects[: i + 1] if not oeff.manual] + ) + ) if oeffect.size: assert not oeffect.manual, "Manual array effects not yet supported" dst = StackEffect(f"&PEEK({osize})", "PyObject **") @@ -674,7 +691,9 @@ def analyze_super(self, super: parser.Super) -> SuperInstruction: parts.append(part) format += instr.instr_fmt final_sp = sp - return SuperInstruction(super.name, stack, initial_sp, final_sp, format, super, parts) + return SuperInstruction( + super.name, stack, initial_sp, final_sp, format, super, parts + ) def analyze_macro(self, macro: parser.Macro) -> MacroInstruction: components = self.check_macro_components(macro) @@ -700,7 +719,9 @@ def analyze_macro(self, macro: parser.Macro) -> MacroInstruction: case _: typing.assert_never(component) final_sp = sp - return MacroInstruction(macro.name, stack, initial_sp, final_sp, format, macro, parts) + return MacroInstruction( + macro.name, stack, initial_sp, final_sp, format, macro, parts + ) def analyze_instruction( self, instr: Instruction, stack: list[StackEffect], sp: int @@ -753,7 +774,9 @@ def stack_analysis( for thing in components: match thing: case Instruction() as instr: - if any(eff.size for eff in instr.input_effects + instr.output_effects): + if any( + eff.size for eff in instr.input_effects + instr.output_effects + ): # TODO: Eventually this will be needed, at least for macros. self.error( f"Instruction {instr.name!r} has variable-sized stack effect, " @@ -779,10 +802,9 @@ def stack_analysis( def get_stack_effect_info( self, thing: parser.InstDef | parser.Super | parser.Macro - ) -> tuple[Instruction|None, str, str]: - + ) -> tuple[Instruction | None, str, str]: def effect_str(effects: list[StackEffect]) -> str: - if getattr(thing, 'kind', None) == 'legacy': + if getattr(thing, "kind", None) == "legacy": return str(-1) n_effect, sym_effect = list_effect_size(effects) if sym_effect: @@ -800,13 +822,21 @@ def effect_str(effects: list[StackEffect]) -> str: popped = pushed = "", "" case parser.Super(): instr = self.super_instrs[thing.name] - popped = '+'.join(effect_str(comp.instr.input_effects) for comp in instr.parts) - pushed = '+'.join(effect_str(comp.instr.output_effects) for comp in instr.parts) + popped = "+".join( + effect_str(comp.instr.input_effects) for comp in instr.parts + ) + pushed = "+".join( + effect_str(comp.instr.output_effects) for comp in instr.parts + ) case parser.Macro(): instr = self.macro_instrs[thing.name] parts = [comp for comp in instr.parts if isinstance(comp, Component)] - popped = '+'.join(effect_str(comp.instr.input_effects) for comp in parts) - pushed = '+'.join(effect_str(comp.instr.output_effects) for comp in parts) + popped = "+".join( + effect_str(comp.instr.input_effects) for comp in parts + ) + pushed = "+".join( + effect_str(comp.instr.output_effects) for comp in parts + ) case _: typing.assert_never(thing) return instr, popped, pushed @@ -817,14 +847,14 @@ def write_stack_effect_functions(self) -> None: for thing in self.everything: instr, popped, pushed = self.get_stack_effect_info(thing) if instr is not None: - popped_data.append( (instr, popped) ) - pushed_data.append( (instr, pushed) ) + popped_data.append((instr, popped)) + pushed_data.append((instr, pushed)) def write_function(direction: str, data: list[tuple[Instruction, str]]) -> None: - self.out.emit("\n#ifndef NDEBUG"); - self.out.emit("static int"); + self.out.emit("\n#ifndef NDEBUG") + self.out.emit("static int") self.out.emit(f"_PyOpcode_num_{direction}(int opcode, int oparg) {{") - self.out.emit(" switch(opcode) {"); + self.out.emit(" switch(opcode) {") for instr, effect in data: self.out.emit(f" case {instr.name}:") self.out.emit(f" return {effect};") @@ -832,10 +862,10 @@ def write_function(direction: str, data: list[tuple[Instruction, str]]) -> None: self.out.emit(" Py_UNREACHABLE();") self.out.emit(" }") self.out.emit("}") - self.out.emit("#endif"); + self.out.emit("#endif") - write_function('popped', popped_data) - write_function('pushed', pushed_data) + write_function("popped", popped_data) + write_function("pushed", pushed_data) def write_metadata(self) -> None: """Write instruction metadata to output file.""" @@ -908,21 +938,21 @@ def write_metadata_for_inst(self, instr: Instruction) -> None: directions.extend("DIR_NONE" for _ in range(3)) dir_op1, dir_op2, dir_op3 = directions[:3] self.out.emit( - f' [{instr.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{instr.instr_fmt} }},' + f" [{instr.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{instr.instr_fmt} }}," ) def write_metadata_for_super(self, sup: SuperInstruction) -> None: """Write metadata for a super-instruction.""" dir_op1 = dir_op2 = dir_op3 = "DIR_NONE" self.out.emit( - f' [{sup.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{sup.instr_fmt} }},' + f" [{sup.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{sup.instr_fmt} }}," ) def write_metadata_for_macro(self, mac: MacroInstruction) -> None: """Write metadata for a macro-instruction.""" dir_op1 = dir_op2 = dir_op3 = "DIR_NONE" self.out.emit( - f' [{mac.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{mac.instr_fmt} }},' + f" [{mac.name}] = {{ {dir_op1}, {dir_op2}, {dir_op3}, true, {INSTR_FMT_PREFIX}{mac.instr_fmt} }}," ) def write_instructions(self) -> None: @@ -1055,7 +1085,9 @@ def extract_block_text(block: parser.Block) -> tuple[list[str], list[str]]: # Separate PREDICT(...) macros from end predictions: list[str] = [] - while blocklines and (m := re.match(r"^\s*PREDICT\((\w+)\);\s*(?://.*)?$", blocklines[-1])): + while blocklines and ( + m := re.match(r"^\s*PREDICT\((\w+)\);\s*(?://.*)?$", blocklines[-1]) + ): predictions.insert(0, m.group(1)) blocklines.pop() @@ -1072,13 +1104,22 @@ def always_exits(lines: list[str]) -> bool: return False line = line[12:] return line.startswith( - ("goto ", "return ", "DISPATCH", "GO_TO_", "Py_UNREACHABLE()", "ERROR_IF(true, ") + ( + "goto ", + "return ", + "DISPATCH", + "GO_TO_", + "Py_UNREACHABLE()", + "ERROR_IF(true, ", + ) ) def variable_used(node: parser.Node, name: str) -> bool: """Determine whether a variable with a given name is used in a node.""" - return any(token.kind == "IDENTIFIER" and token.text == name for token in node.tokens) + return any( + token.kind == "IDENTIFIER" and token.text == name for token in node.tokens + ) def main(): From 62409346ce4edef34ee63b2591c42e1a3a627284 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 26 Jan 2023 11:57:25 -0800 Subject: [PATCH 08/13] Use Irit's idea --- Python/generated_cases.c.h | 7 +-- Tools/cases_generator/generate_cases.py | 61 ++++++++----------------- Tools/cases_generator/parser.py | 2 - Tools/cases_generator/test_generator.py | 26 +++++++---- 4 files changed, 39 insertions(+), 57 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 7278f8a351cbb0..ceae0a30553be7 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1794,8 +1794,9 @@ Py_DECREF(owner); if (res == NULL) goto pop_1_error; } - POKE(1, res); - if (oparg & 1) { PUSH(res2); } + STACK_GROW(((oparg & 1) != 0)); + if (oparg & 1) { POKE(((oparg & 1) != 0), res2); } + POKE(1 + ((oparg & 1) != 0), res); JUMPBY(9); DISPATCH(); } @@ -2268,8 +2269,8 @@ if (!Py_IsNone(match)) { PyErr_SetExcInfo(NULL, Py_NewRef(match), NULL); } - POKE(2, rest); POKE(1, match); + POKE(2, rest); DISPATCH(); } diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index c208b65deeb5e3..110af920494cf4 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -168,16 +168,21 @@ def declare(self, dst: StackEffect, src: StackEffect | None): self.emit(f"{typ}{sepa}{dst.name}{init};") def assign(self, dst: StackEffect, src: StackEffect): + if "boerenkool" in dst.name or "boerenkool" in src.name: + breakpoint() if src.name == UNUSED: return cast = self.cast(dst, src) if m := re.match(r"^PEEK\((.*)\)$", dst.name): - self.emit(f"POKE({m.group(1)}, {cast}{src.name});") - elif m := re.match(r"^POP\(\)$", dst.name): + stmt = f"POKE({m.group(1)}, {cast}{src.name});" if src.cond: - self.emit(f"if ({src.cond}) {{ PUSH({cast}{src.name}); }}") - else: - self.emit(f"PUSH({cast}{src.name});") + stmt = f"if ({src.cond}) {{ {stmt} }}" + self.emit(stmt) + # elif m := re.match(r"^POP\(\)$", dst.name): + # if src.cond: + # self.emit(f"if ({src.cond}) {{ PUSH({cast}{src.name}); }}") + # else: + # self.emit(f"PUSH({cast}{src.name});") elif m := re.match(r"^&PEEK\(.*\)$", dst.name): # NOTE: MOVE_ITEMS() does not actually exist. # The only supported output array forms are: @@ -235,23 +240,10 @@ def __init__(self, inst: parser.InstDef): effect for effect in inst.inputs if isinstance(effect, parser.CacheEffect) ] self.cache_offset = sum(c.size for c in self.cache_effects) - self.input_effects = [ effect for effect in inst.inputs if isinstance(effect, StackEffect) ] - manual = False - for effect in self.input_effects: - if effect.cond: - manual = True - effect.manual = manual - self.output_effects = inst.outputs # For consistency/completeness - manual = False - for effect in self.output_effects: - if effect.cond: - manual = True - effect.manual = manual - unmoved_names: set[str] = set() for ieffect, oeffect in zip(self.input_effects, self.output_effects): if ieffect.name == oeffect.name: @@ -306,28 +298,19 @@ def write(self, out: Formatter) -> None: for i, ieffect in enumerate(ieffects): isize = string_effect_size( list_effect_size( - [ieff for ieff in ieffects[: i + 1] if not ieff.manual] + [ieff for ieff in ieffects[: i + 1]] ) ) if ieffect.size: - assert not ieffect.manual, "Manual array effects not yet supported" src = StackEffect(f"&PEEK({isize})", "PyObject **") + elif ieffect.cond: + src = StackEffect(f"({ieffect.cond}) ? PEEK({isize}) : NULL", "") else: - if ieffect.manual: - if ieffect.cond: - src = StackEffect(f"({ieffect.cond}) ? POP() : NULL", "") - else: - src = StackEffect(f"POP()", "") - else: - src = StackEffect(f"PEEK({isize})", "") + src = StackEffect(f"PEEK({isize})", "") out.declare(ieffect, src) - # if ieffect.cond: - # assert ieffect.manual, "Conditional effects must be manual" - # out.emit(f"if ({ieffect.cond}) {{ {ieffect.name} = POP(); }}") else: # Write input register variable declarations and initializations for ieffect, reg in zip(self.input_effects, self.input_registers): - assert not ieffect.manual, "Manual register effects not yet supported" src = StackEffect(reg, "") out.declare(ieffect, src) @@ -349,36 +332,28 @@ def write(self, out: Formatter) -> None: # Write net stack growth/shrinkage out.stack_adjust( 0, - [ieff for ieff in self.input_effects if not ieff.manual], - [oeff for oeff in self.output_effects if not oeff.manual], + [ieff for ieff in self.input_effects], + [oeff for oeff in self.output_effects], ) # Write output stack effect assignments oeffects = list(reversed(self.output_effects)) - real_oeffects: list[tuple[StackEffect, StackEffect]] = [] for i, oeffect in enumerate(oeffects): if oeffect.name in self.unmoved_names: continue osize = string_effect_size( list_effect_size( - [oeff for oeff in oeffects[: i + 1] if not oeff.manual] + [oeff for oeff in oeffects[: i + 1]] ) ) if oeffect.size: - assert not oeffect.manual, "Manual array effects not yet supported" dst = StackEffect(f"&PEEK({osize})", "PyObject **") else: - if oeffect.manual: - dst = StackEffect(f"POP()", "") - else: - dst = StackEffect(f"PEEK({osize})", "") - real_oeffects.append((dst, oeffect)) - for dst, oeffect in reversed(real_oeffects): + dst = StackEffect(f"PEEK({osize})", "") out.assign(dst, oeffect) else: # Write output register assignments for oeffect, reg in zip(self.output_effects, self.output_registers): - assert not oeffect.manual, "Manual register effects not yet supported" src = StackEffect(reg, "") dst = StackEffect(reg, "") out.assign(dst, oeffect) diff --git a/Tools/cases_generator/parser.py b/Tools/cases_generator/parser.py index 1c0dd8b6ef9f58..ced66faee4931f 100644 --- a/Tools/cases_generator/parser.py +++ b/Tools/cases_generator/parser.py @@ -74,8 +74,6 @@ class StackEffect(Node): cond: str = "" # Optional `if (cond)` size: str = "" # Optional `[size]` # Note: size cannot be combined with type or cond - # 'manual' is used to decide between PEEK() and POP() - manual: bool = field(init=False, compare=False, default=False) @dataclass diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index 224d00bbfb2cd2..8a8a77e3d978ba 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -54,6 +54,12 @@ def run_cases_test(input: str, expected: str): while lines and lines[0].startswith("// "): lines.pop(0) actual = "".join(lines) + # if actual.rstrip() != expected.rstrip(): + # print("Actual:") + # print(actual) + # print("Expected:") + # print(expected) + # print("End") assert actual.rstrip() == expected.rstrip() def test_legacy(): @@ -411,9 +417,9 @@ def test_array_output(): spam(); STACK_GROW(2); STACK_GROW(oparg*3); - POKE(2 + oparg*3, below); - MOVE_ITEMS(&PEEK(1 + oparg*3), values, oparg*3); POKE(1, above); + MOVE_ITEMS(&PEEK(1 + oparg*3), values, oparg*3); + POKE(2 + oparg*3, below); DISPATCH(); } """ @@ -431,8 +437,8 @@ def test_array_input_output(): PyObject *below = PEEK(1 + oparg); PyObject *above; spam(); - MOVE_ITEMS(&PEEK(1 + oparg), values, oparg); POKE(1, above); + MOVE_ITEMS(&PEEK(1 + oparg), values, oparg); DISPATCH(); } """ @@ -484,16 +490,18 @@ def test_cond_effect(): """ output = """ TARGET(OP) { - PyObject *cc = POP(); - PyObject *input = (oparg & 1) ? POP() : NULL; - PyObject *aa = PEEK(1); + PyObject *cc = PEEK(1); + PyObject *input = (oparg & 1) ? PEEK(1 + ((oparg & 1) != 0)) : NULL; + PyObject *aa = PEEK(2 + ((oparg & 1) != 0)); PyObject *xx; PyObject *output; PyObject *zz; output = spam(oparg, input); - POKE(1, xx); - if (oparg & 2) { PUSH(output); } - PUSH(zz); + STACK_SHRINK(((oparg & 1) != 0)); + STACK_GROW(((oparg & 2) != 0)); + POKE(1, zz); + if (oparg & 2) { POKE(1 + ((oparg & 2) != 0), output); } + POKE(2 + ((oparg & 2) != 0), xx); DISPATCH(); } """ From d78e2a74a7f2c51c3bd93e8728c4fd550a596605 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 26 Jan 2023 15:53:12 -0800 Subject: [PATCH 09/13] Cleanup --- Tools/cases_generator/generate_cases.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 110af920494cf4..a7fe15ed005d3d 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -61,7 +61,7 @@ def effect_size(effect: StackEffect) -> tuple[int, str]: At most one of these will be non-zero / non-empty. """ if effect.size: - assert not effect.cond, "Manual effects should be conditional" + assert not effect.cond, "Array effects cannot have a condition" return 0, effect.size elif effect.cond: return 0, f"{maybe_parenthesize(effect.cond)} != 0" @@ -168,8 +168,6 @@ def declare(self, dst: StackEffect, src: StackEffect | None): self.emit(f"{typ}{sepa}{dst.name}{init};") def assign(self, dst: StackEffect, src: StackEffect): - if "boerenkool" in dst.name or "boerenkool" in src.name: - breakpoint() if src.name == UNUSED: return cast = self.cast(dst, src) @@ -178,11 +176,6 @@ def assign(self, dst: StackEffect, src: StackEffect): if src.cond: stmt = f"if ({src.cond}) {{ {stmt} }}" self.emit(stmt) - # elif m := re.match(r"^POP\(\)$", dst.name): - # if src.cond: - # self.emit(f"if ({src.cond}) {{ PUSH({cast}{src.name}); }}") - # else: - # self.emit(f"PUSH({cast}{src.name});") elif m := re.match(r"^&PEEK\(.*\)$", dst.name): # NOTE: MOVE_ITEMS() does not actually exist. # The only supported output array forms are: @@ -297,9 +290,7 @@ def write(self, out: Formatter) -> None: ieffects = list(reversed(self.input_effects)) for i, ieffect in enumerate(ieffects): isize = string_effect_size( - list_effect_size( - [ieff for ieff in ieffects[: i + 1]] - ) + list_effect_size([ieff for ieff in ieffects[: i + 1]]) ) if ieffect.size: src = StackEffect(f"&PEEK({isize})", "PyObject **") @@ -342,9 +333,7 @@ def write(self, out: Formatter) -> None: if oeffect.name in self.unmoved_names: continue osize = string_effect_size( - list_effect_size( - [oeff for oeff in oeffects[: i + 1]] - ) + list_effect_size([oeff for oeff in oeffects[: i + 1]]) ) if oeffect.size: dst = StackEffect(f"&PEEK({osize})", "PyObject **") @@ -354,7 +343,6 @@ def write(self, out: Formatter) -> None: else: # Write output register assignments for oeffect, reg in zip(self.output_effects, self.output_registers): - src = StackEffect(reg, "") dst = StackEffect(reg, "") out.assign(dst, oeffect) From e69c1f3e7a01d253e05bd01ce0b57dce9b96cfeb Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 26 Jan 2023 16:42:59 -0800 Subject: [PATCH 10/13] Change order in LOAD_ATTR output effect --- Python/bytecodes.c | 10 +++++----- Python/generated_cases.c.h | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index c7b8e233443374..fb00b887732ed0 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1438,7 +1438,7 @@ dummy_func( PREDICT(JUMP_BACKWARD); } - inst(LOAD_ATTR, (unused/9, owner -- res, res2 if (oparg & 1))) { + inst(LOAD_ATTR, (unused/9, owner -- res2 if (oparg & 1), res)) { #if ENABLE_SPECIALIZATION _PyAttrCache *cache = (_PyAttrCache *)next_instr; if (ADAPTIVE_COUNTER_IS_ZERO(cache->counter)) { @@ -1462,8 +1462,8 @@ dummy_func( meth | self | arg1 | ... | argN */ assert(meth != NULL); // No errors on this branch - res = meth; - res2 = owner; // Transfer ownership + res2 = meth; + res = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -1475,8 +1475,8 @@ dummy_func( */ Py_DECREF(owner); ERROR_IF(meth == NULL, error); - res = NULL; - res2 = meth; + res2 = NULL; + res = meth; } } else { diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index ceae0a30553be7..93adac6472d437 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1746,8 +1746,8 @@ TARGET(LOAD_ATTR) { PREDICTED(LOAD_ATTR); PyObject *owner = PEEK(1); - PyObject *res; PyObject *res2; + PyObject *res; #if ENABLE_SPECIALIZATION _PyAttrCache *cache = (_PyAttrCache *)next_instr; if (ADAPTIVE_COUNTER_IS_ZERO(cache->counter)) { @@ -1771,8 +1771,8 @@ meth | self | arg1 | ... | argN */ assert(meth != NULL); // No errors on this branch - res = meth; - res2 = owner; // Transfer ownership + res2 = meth; + res = owner; // Transfer ownership } else { /* meth is not an unbound method (but a regular attr, or @@ -1784,8 +1784,8 @@ */ Py_DECREF(owner); if (meth == NULL) goto pop_1_error; - res = NULL; - res2 = meth; + res2 = NULL; + res = meth; } } else { @@ -1795,8 +1795,8 @@ if (res == NULL) goto pop_1_error; } STACK_GROW(((oparg & 1) != 0)); - if (oparg & 1) { POKE(((oparg & 1) != 0), res2); } - POKE(1 + ((oparg & 1) != 0), res); + POKE(1, res); + if (oparg & 1) { POKE(1 + ((oparg & 1) != 0), res2); } JUMPBY(9); DISPATCH(); } From c5529c66c080781f9003de6b312ed3ef2d663500 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Fri, 27 Jan 2023 18:27:23 -0800 Subject: [PATCH 11/13] Fix type annotations to satisfy pyright (and mypy mostly) --- Tools/cases_generator/generate_cases.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index a7fe15ed005d3d..935ecc8ac4e667 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -461,6 +461,7 @@ class MacroInstruction(SuperOrMacroInstruction): parts: list[Component | parser.CacheEffect] +AnyInstruction = Instruction | SuperInstruction | MacroInstruction INSTR_FMT_PREFIX = "INSTR_FMT_" @@ -529,6 +530,7 @@ def parse(self) -> None: self.supers = {} self.macros = {} self.families = {} + thing: parser.InstDef | parser.Super | parser.Macro | parser.Family | None while thing := psr.definition(): match thing: case parser.InstDef(name=name): @@ -765,7 +767,7 @@ def stack_analysis( def get_stack_effect_info( self, thing: parser.InstDef | parser.Super | parser.Macro - ) -> tuple[Instruction | None, str, str]: + ) -> tuple[AnyInstruction | None, str, str]: def effect_str(effects: list[StackEffect]) -> str: if getattr(thing, "kind", None) == "legacy": return str(-1) @@ -774,6 +776,7 @@ def effect_str(effects: list[StackEffect]) -> str: return f"{sym_effect} + {n_effect}" if n_effect else sym_effect return str(n_effect) + instr: AnyInstruction | None match thing: case parser.InstDef(): if thing.kind != "op": @@ -782,7 +785,8 @@ def effect_str(effects: list[StackEffect]) -> str: pushed = effect_str(instr.output_effects) else: instr = None - popped = pushed = "", "" + popped = "" + pushed = "" case parser.Super(): instr = self.super_instrs[thing.name] popped = "+".join( @@ -805,15 +809,15 @@ def effect_str(effects: list[StackEffect]) -> str: return instr, popped, pushed def write_stack_effect_functions(self) -> None: - popped_data = [] - pushed_data = [] + popped_data: list[tuple[AnyInstruction, str]] = [] + pushed_data: list[tuple[AnyInstruction, str]] = [] for thing in self.everything: instr, popped, pushed = self.get_stack_effect_info(thing) if instr is not None: popped_data.append((instr, popped)) pushed_data.append((instr, pushed)) - def write_function(direction: str, data: list[tuple[Instruction, str]]) -> None: + def write_function(direction: str, data: list[tuple[AnyInstruction, str]]) -> None: self.out.emit("\n#ifndef NDEBUG") self.out.emit("static int") self.out.emit(f"_PyOpcode_num_{direction}(int opcode, int oparg) {{") From 37e3b407f26160c20cbbd85fd55489ce237b8946 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Sat, 28 Jan 2023 15:09:57 -0800 Subject: [PATCH 12/13] Solve uninitalized variable warning using = NULL --- Python/generated_cases.c.h | 2 +- Tools/cases_generator/generate_cases.py | 5 ++++- Tools/cases_generator/test_generator.py | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 93adac6472d437..4ea45866901b0e 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1746,7 +1746,7 @@ TARGET(LOAD_ATTR) { PREDICTED(LOAD_ATTR); PyObject *owner = PEEK(1); - PyObject *res2; + PyObject *res2 = NULL; PyObject *res; #if ENABLE_SPECIALIZATION _PyAttrCache *cache = (_PyAttrCache *)next_instr; diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 935ecc8ac4e667..007153f3953229 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -160,10 +160,13 @@ def declare(self, dst: StackEffect, src: StackEffect | None): if dst.name == UNUSED: return typ = f"{dst.type}" if dst.type else "PyObject *" - init = "" if src: cast = self.cast(dst, src) init = f" = {cast}{src.name}" + elif dst.cond: + init = " = NULL" + else: + init = "" sepa = "" if typ.endswith("*") else " " self.emit(f"{typ}{sepa}{dst.name}{init};") diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index 8a8a77e3d978ba..360e8a80e685c4 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -494,7 +494,7 @@ def test_cond_effect(): PyObject *input = (oparg & 1) ? PEEK(1 + ((oparg & 1) != 0)) : NULL; PyObject *aa = PEEK(2 + ((oparg & 1) != 0)); PyObject *xx; - PyObject *output; + PyObject *output = NULL; PyObject *zz; output = spam(oparg, input); STACK_SHRINK(((oparg & 1) != 0)); From 81e523659245b8fb421aa755ac87abc44fa4ed98 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Sat, 28 Jan 2023 15:13:55 -0800 Subject: [PATCH 13/13] Use 'X ? 1 : 0' instead of 'X != 0' This hopefully avoids a silly compiler warning. --- Python/generated_cases.c.h | 4 ++-- Python/opcode_metadata.h | 2 +- Tools/cases_generator/generate_cases.py | 2 +- Tools/cases_generator/test_generator.py | 12 ++++++------ 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 4ea45866901b0e..b5decf804ca652 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1794,9 +1794,9 @@ Py_DECREF(owner); if (res == NULL) goto pop_1_error; } - STACK_GROW(((oparg & 1) != 0)); + STACK_GROW(((oparg & 1) ? 1 : 0)); POKE(1, res); - if (oparg & 1) { POKE(1 + ((oparg & 1) != 0), res2); } + if (oparg & 1) { POKE(1 + ((oparg & 1) ? 1 : 0), res2); } JUMPBY(9); DISPATCH(); } diff --git a/Python/opcode_metadata.h b/Python/opcode_metadata.h index 568a0b0844016e..e76ddda2f0292d 100644 --- a/Python/opcode_metadata.h +++ b/Python/opcode_metadata.h @@ -531,7 +531,7 @@ _PyOpcode_num_pushed(int opcode, int oparg) { case MAP_ADD: return 0; case LOAD_ATTR: - return ((oparg & 1) != 0) + 1; + return ((oparg & 1) ? 1 : 0) + 1; case LOAD_ATTR_INSTANCE_VALUE: return -1; case LOAD_ATTR_MODULE: diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 007153f3953229..1d703a0a790ed5 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -64,7 +64,7 @@ def effect_size(effect: StackEffect) -> tuple[int, str]: assert not effect.cond, "Array effects cannot have a condition" return 0, effect.size elif effect.cond: - return 0, f"{maybe_parenthesize(effect.cond)} != 0" + return 0, f"{maybe_parenthesize(effect.cond)} ? 1 : 0" else: return 1, "" diff --git a/Tools/cases_generator/test_generator.py b/Tools/cases_generator/test_generator.py index 360e8a80e685c4..6e6c60782d73d3 100644 --- a/Tools/cases_generator/test_generator.py +++ b/Tools/cases_generator/test_generator.py @@ -491,17 +491,17 @@ def test_cond_effect(): output = """ TARGET(OP) { PyObject *cc = PEEK(1); - PyObject *input = (oparg & 1) ? PEEK(1 + ((oparg & 1) != 0)) : NULL; - PyObject *aa = PEEK(2 + ((oparg & 1) != 0)); + PyObject *input = (oparg & 1) ? PEEK(1 + ((oparg & 1) ? 1 : 0)) : NULL; + PyObject *aa = PEEK(2 + ((oparg & 1) ? 1 : 0)); PyObject *xx; PyObject *output = NULL; PyObject *zz; output = spam(oparg, input); - STACK_SHRINK(((oparg & 1) != 0)); - STACK_GROW(((oparg & 2) != 0)); + STACK_SHRINK(((oparg & 1) ? 1 : 0)); + STACK_GROW(((oparg & 2) ? 1 : 0)); POKE(1, zz); - if (oparg & 2) { POKE(1 + ((oparg & 2) != 0), output); } - POKE(2 + ((oparg & 2) != 0), xx); + if (oparg & 2) { POKE(1 + ((oparg & 2) ? 1 : 0), output); } + POKE(2 + ((oparg & 2) ? 1 : 0), xx); DISPATCH(); } """