From 87eed1237d2056de6f66ac95fa379533e94c3cf2 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Mon, 21 Aug 2023 17:10:52 -0700 Subject: [PATCH 1/5] Fix bug in copy detection I was comparing the last preceding poke with the *last* peek, rather than the *first* peek. Unfortunately this bug obscured another bug, which I have yet to fix: When the last preceding poke is UNUSED, the first peek disappears, leaving the variable unassigned. --- Tools/cases_generator/stacking.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index 1e117f11825938..17fe4583f15615 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -179,7 +179,7 @@ def __init__( while ( pred.pokes and self.peeks - and pred.pokes[-1].effect == self.peeks[-1].effect + and pred.pokes[-1].effect == self.peeks[0].effect ): src = pred.pokes.pop(-1).effect dst = self.peeks.pop(0).effect From de494fceaa092ea647ae158e28868a02018a7312 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 23 Aug 2023 14:01:28 -0700 Subject: [PATCH 2/5] Fix problem with copying from 'unused' Rename CopyEffect to CopyItem. Change CopyItem to contain StackItems instead of StackEffects. Update those StackItems when adjusting the manager higher or lower. Assert that those StackItems' offsets are equivalent. --- Python/generated_cases.c.h | 2 - Tools/cases_generator/stacking.py | 65 +++++++++++++++++++++++-------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index f6322df566c650..693fd0fceb9e33 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -3833,7 +3833,6 @@ DEOPT_IF(code->co_argcount != oparg + (self_or_null != NULL), CALL); } // _CHECK_STACK_SPACE - callable = stack_pointer[-2 - oparg]; { PyFunctionObject *func = (PyFunctionObject *)callable; PyCodeObject *code = (PyCodeObject *)func->func_code; @@ -3842,7 +3841,6 @@ // _INIT_CALL_PY_EXACT_ARGS args = stack_pointer - oparg; self_or_null = stack_pointer[-1 - oparg]; - callable = stack_pointer[-2 - oparg]; { int argcount = oparg; if (self_or_null != NULL) { diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index 17fe4583f15615..fd1b189d392d4d 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -83,6 +83,27 @@ def as_index(self) -> str: terms = self.as_terms() return make_index(terms) + def equivalent_to(self, other: "StackOffset") -> bool: + if self.deep == other.deep and self.high == other.high: + return True + deep = list(self.deep) + for x in other.deep: + try: + deep.remove(x) + except ValueError: + return False + if deep: + return False + high = list(self.high) + for x in other.high: + try: + high.remove(x) + except ValueError: + return False + if high: + return False + return True + def make_index(terms: list[tuple[str, str]]) -> str: # Produce an index expression from the terms honoring PEP 8, @@ -131,9 +152,9 @@ def as_stack_effect(self, lax: bool = False) -> StackEffect: @dataclasses.dataclass -class CopyEffect: - src: StackEffect - dst: StackEffect +class CopyItem: + src: StackItem + dst: StackItem class EffectManager: @@ -143,7 +164,7 @@ class EffectManager: active_caches: list[ActiveCacheEffect] peeks: list[StackItem] pokes: list[StackItem] - copies: list[CopyEffect] # See merge() + copies: list[CopyItem] # See merge() # Track offsets from stack pointer min_offset: StackOffset final_offset: StackOffset @@ -181,14 +202,17 @@ def __init__( and self.peeks and pred.pokes[-1].effect == self.peeks[0].effect ): - src = pred.pokes.pop(-1).effect - dst = self.peeks.pop(0).effect - pred.final_offset.deeper(src) - if dst.name != UNUSED: - destinations.add(dst.name) - if dst.name != src.name: - sources.add(src.name) - self.copies.append(CopyEffect(src, dst)) + src = pred.pokes.pop(-1) + dst = self.peeks.pop(0) + src_offset = src.offset + dst_offset = dst.offset + assert src.offset.equivalent_to(dst.offset), (src, dst) + pred.final_offset.deeper(src.effect) + if dst.effect.name != UNUSED: + destinations.add(dst.effect.name) + if dst.effect.name != src.effect.name: + sources.add(src.effect.name) + self.copies.append(CopyItem(src, dst)) # TODO: Turn this into an error (pass an Analyzer instance?) assert sources & destinations == set(), ( pred.instr.name, @@ -207,6 +231,9 @@ def adjust_deeper(self, eff: StackEffect) -> None: peek.offset.deeper(eff) for poke in self.pokes: poke.offset.deeper(eff) + for copy in self.copies: + copy.src.offset.deeper(eff) + copy.dst.offset.deeper(eff) self.min_offset.deeper(eff) self.final_offset.deeper(eff) @@ -215,6 +242,9 @@ def adjust_higher(self, eff: StackEffect) -> None: peek.offset.higher(eff) for poke in self.pokes: poke.offset.higher(eff) + for copy in self.copies: + copy.src.offset.higher(eff) + copy.dst.offset.higher(eff) self.min_offset.higher(eff) self.final_offset.higher(eff) @@ -248,8 +278,8 @@ def add(eff: StackEffect) -> None: vars[eff.name] = eff for copy in self.copies: - add(copy.src) - add(copy.dst) + add(copy.src.effect) + add(copy.dst.effect) for peek in self.peeks: add(peek.effect) for poke in self.pokes: @@ -365,8 +395,11 @@ def write_components( out.emit(f"// {mgr.instr.name}") for copy in mgr.copies: - if copy.src.name != copy.dst.name: - out.assign(copy.dst, copy.src) + copy_src_effect = copy.src.effect + if copy_src_effect.name != copy.dst.effect.name: + if copy_src_effect.name == UNUSED: + copy_src_effect = copy.src.as_stack_effect() + out.assign(copy.dst.effect, copy_src_effect) for peek in mgr.peeks: out.assign( peek.effect, From 848822f38cfca4356fb8fe0c57a8d06faaab0f99 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 23 Aug 2023 17:30:38 -0700 Subject: [PATCH 3/5] Do more clever things with copies --- Python/generated_cases.c.h | 1 - Tools/cases_generator/stacking.py | 25 +++++++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 693fd0fceb9e33..1724d11231727f 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -3840,7 +3840,6 @@ } // _INIT_CALL_PY_EXACT_ARGS args = stack_pointer - oparg; - self_or_null = stack_pointer[-1 - oparg]; { int argcount = oparg; if (self_or_null != NULL) { diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index fd1b189d392d4d..d4eafd69c21b84 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -204,13 +204,12 @@ def __init__( ): src = pred.pokes.pop(-1) dst = self.peeks.pop(0) - src_offset = src.offset - dst_offset = dst.offset assert src.offset.equivalent_to(dst.offset), (src, dst) pred.final_offset.deeper(src.effect) - if dst.effect.name != UNUSED: - destinations.add(dst.effect.name) - if dst.effect.name != src.effect.name: + if dst.effect.name != src.effect.name: + if dst.effect.name != UNUSED: + destinations.add(dst.effect.name) + if src.effect.name != UNUSED: sources.add(src.effect.name) self.copies.append(CopyItem(src, dst)) # TODO: Turn this into an error (pass an Analyzer instance?) @@ -226,6 +225,18 @@ def __init__( else: pred = None # Break + # Fix up patterns of copies through UNUSED, + # e.g. cp(a, UNUSED) + cp(UNUSED, b) -> cp(a, b). + if any(copy.src.effect.name == UNUSED for copy in self.copies): + pred = self + while pred := pred.pred: + for copy in self.copies: + if copy.src.effect.name == UNUSED: + for pred_copy in pred.copies: + if pred_copy.dst == copy.src: + copy.src = pred_copy.src + break + def adjust_deeper(self, eff: StackEffect) -> None: for peek in self.peeks: peek.offset.deeper(eff) @@ -399,7 +410,9 @@ def write_components( if copy_src_effect.name != copy.dst.effect.name: if copy_src_effect.name == UNUSED: copy_src_effect = copy.src.as_stack_effect() - out.assign(copy.dst.effect, copy_src_effect) + out.assign(copy.dst.effect, copy_src_effect) + else: + out.assign(copy.dst.effect, copy_src_effect) for peek in mgr.peeks: out.assign( peek.effect, From 1ca845aae0057b138b8928d75d1afed8a4aa0ea3 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 23 Aug 2023 19:01:13 -0700 Subject: [PATCH 4/5] Fix mypy --- Tools/cases_generator/stacking.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index d4eafd69c21b84..165e64d177b1c5 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -228,14 +228,15 @@ def __init__( # Fix up patterns of copies through UNUSED, # e.g. cp(a, UNUSED) + cp(UNUSED, b) -> cp(a, b). if any(copy.src.effect.name == UNUSED for copy in self.copies): - pred = self - while pred := pred.pred: + pred = self.pred + while pred is not None: for copy in self.copies: if copy.src.effect.name == UNUSED: for pred_copy in pred.copies: if pred_copy.dst == copy.src: copy.src = pred_copy.src break + pred = pred.pred def adjust_deeper(self, eff: StackEffect) -> None: for peek in self.peeks: From 652f9a16041148a8b793e5816514d55ca8888414 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Thu, 24 Aug 2023 11:27:34 -0700 Subject: [PATCH 5/5] Reduce complexity (I had different code in one of the branches during debugging. :-) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- Tools/cases_generator/stacking.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Tools/cases_generator/stacking.py b/Tools/cases_generator/stacking.py index 165e64d177b1c5..2d006ba8e5934a 100644 --- a/Tools/cases_generator/stacking.py +++ b/Tools/cases_generator/stacking.py @@ -411,9 +411,7 @@ def write_components( if copy_src_effect.name != copy.dst.effect.name: if copy_src_effect.name == UNUSED: copy_src_effect = copy.src.as_stack_effect() - out.assign(copy.dst.effect, copy_src_effect) - else: - out.assign(copy.dst.effect, copy_src_effect) + out.assign(copy.dst.effect, copy_src_effect) for peek in mgr.peeks: out.assign( peek.effect,