From b8b261aa8a02a44d234f3c315a892c38f41b95bb Mon Sep 17 00:00:00 2001 From: Hodan Date: Fri, 30 Aug 2024 15:31:34 +0200 Subject: [PATCH 1/8] fix[venom]: stack reorder rework --- vyper/venom/venom_to_assembly.py | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index 07d63afc70..b3bc97b747 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -1,4 +1,5 @@ from collections import Counter +from collections import defaultdict from typing import Any from vyper.exceptions import CompilerPanic, StackTooDeep @@ -209,12 +210,18 @@ def _stack_reorder( stack_ops_count = len(stack_ops) counts = Counter(stack_ops) + + positions : dict[IROperand, list[int]]= defaultdict(lambda : []) + for op in stack_ops: + positions[op] = [] + for i in range(counts[op]): + positions[op].append(stack.get_depth(op, i + 1)) for i in range(stack_ops_count): op = stack_ops[i] final_stack_depth = -(stack_ops_count - i - 1) - depth = stack.get_depth(op, counts[op]) # type: ignore - counts[op] -= 1 + depth = positions[op].pop() # type: ignore + assert depth not in range(-stack_ops_count + 1, final_stack_depth), f"{depth} : ({-stack_ops_count - 1}, {final_stack_depth})" if depth == StackModel.NOT_IN_STACK: raise CompilerPanic(f"Variable {op} not in stack") @@ -225,9 +232,22 @@ def _stack_reorder( if op == stack.peek(final_stack_depth): continue + if len(positions[stack.peek(0)]) != 0: + positions[stack.peek(0)].remove(0) + positions[stack.peek(0)].append(depth) + positions[stack.peek(0)].sort() + cost += self.swap(assembly, stack, depth) + if final_stack_depth in positions[stack.peek(final_stack_depth)]: + positions[stack.peek(final_stack_depth)].remove(final_stack_depth) + positions[stack.peek(final_stack_depth)].insert(0, 0) + else: + positions[stack.peek(final_stack_depth)].insert(0, 0) + cost += self.swap(assembly, stack, final_stack_depth) + assert len(stack_ops) == 0 or stack._stack[-len(stack_ops):] == stack_ops, f"Wrong stack {stack} {stack_ops}" + return cost def _emit_input_operands( From 8db93fa6afdbc4e660bcc8efd35da2aef19f1aa7 Mon Sep 17 00:00:00 2001 From: Hodan Date: Tue, 3 Sep 2024 10:47:41 +0200 Subject: [PATCH 2/8] fix[venom]: stack reorder rework (lint) --- vyper/venom/venom_to_assembly.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index b3bc97b747..bb383e4bef 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -1,5 +1,4 @@ -from collections import Counter -from collections import defaultdict +from collections import Counter, defaultdict from typing import Any from vyper.exceptions import CompilerPanic, StackTooDeep @@ -210,8 +209,8 @@ def _stack_reorder( stack_ops_count = len(stack_ops) counts = Counter(stack_ops) - - positions : dict[IROperand, list[int]]= defaultdict(lambda : []) + + positions: dict[IROperand, list[int]] = defaultdict(lambda: []) for op in stack_ops: positions[op] = [] for i in range(counts[op]): @@ -221,7 +220,9 @@ def _stack_reorder( op = stack_ops[i] final_stack_depth = -(stack_ops_count - i - 1) depth = positions[op].pop() # type: ignore - assert depth not in range(-stack_ops_count + 1, final_stack_depth), f"{depth} : ({-stack_ops_count - 1}, {final_stack_depth})" + assert depth not in range( + -stack_ops_count + 1, final_stack_depth + ), f"{depth} : ({-stack_ops_count - 1}, {final_stack_depth})" if depth == StackModel.NOT_IN_STACK: raise CompilerPanic(f"Variable {op} not in stack") @@ -246,7 +247,9 @@ def _stack_reorder( cost += self.swap(assembly, stack, final_stack_depth) - assert len(stack_ops) == 0 or stack._stack[-len(stack_ops):] == stack_ops, f"Wrong stack {stack} {stack_ops}" + assert ( + len(stack_ops) == 0 or stack._stack[-len(stack_ops) :] == stack_ops + ), f"Wrong stack {stack} {stack_ops}" return cost From aed28e3af30972b01d1d85d102dc8271e2881112 Mon Sep 17 00:00:00 2001 From: Hodan Date: Tue, 3 Sep 2024 11:28:06 +0200 Subject: [PATCH 3/8] fix[venom]: small improvement --- vyper/venom/venom_to_assembly.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index bb383e4bef..3be3facc8d 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -1,3 +1,4 @@ +from bisect import insort from collections import Counter, defaultdict from typing import Any @@ -235,8 +236,7 @@ def _stack_reorder( if len(positions[stack.peek(0)]) != 0: positions[stack.peek(0)].remove(0) - positions[stack.peek(0)].append(depth) - positions[stack.peek(0)].sort() + insort(positions[stack.peek(0)], depth) cost += self.swap(assembly, stack, depth) if final_stack_depth in positions[stack.peek(final_stack_depth)]: From d9614069cd0a7877adb377d6d1956458ac20429f Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 9 Sep 2024 09:31:13 +0200 Subject: [PATCH 4/8] small refactor (factoring out the vars) + bit of documentation --- vyper/venom/venom_to_assembly.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index 3be3facc8d..5b838074da 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -211,6 +211,11 @@ def _stack_reorder( counts = Counter(stack_ops) + # positions stores the positions of relevant operands + # on stack for example operand %82 is on positions [0, 3] + # this operand could ocure even more deeper in the stack + # but only those that are needed/relevant in calculation + # are considered positions: dict[IROperand, list[int]] = defaultdict(lambda: []) for op in stack_ops: positions[op] = [] @@ -234,16 +239,21 @@ def _stack_reorder( if op == stack.peek(final_stack_depth): continue - if len(positions[stack.peek(0)]) != 0: - positions[stack.peek(0)].remove(0) - insort(positions[stack.peek(0)], depth) + # moves the top item to original position + top_item_positions = positions[stack.peek(0)] + if len(top_item_positions) != 0: + top_item_positions.remove(0) + insort(top_item_positions, depth) cost += self.swap(assembly, stack, depth) - if final_stack_depth in positions[stack.peek(final_stack_depth)]: - positions[stack.peek(final_stack_depth)].remove(final_stack_depth) - positions[stack.peek(final_stack_depth)].insert(0, 0) + + # moves the item from final position to top + final_item_positions = positions[stack.peek(final_stack_depth)] + if final_stack_depth in final_item_positions: + final_item_positions.remove(final_stack_depth) + final_item_positions.insert(0, 0) else: - positions[stack.peek(final_stack_depth)].insert(0, 0) + final_item_positions.insert(0, 0) cost += self.swap(assembly, stack, final_stack_depth) From 52b1c41c91cb91ebba688f64bd014bccec8e3e88 Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 9 Sep 2024 11:25:23 +0200 Subject: [PATCH 5/8] stack reorder regression test --- .../unit/compiler/venom/test_stack_reorder.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 tests/unit/compiler/venom/test_stack_reorder.py diff --git a/tests/unit/compiler/venom/test_stack_reorder.py b/tests/unit/compiler/venom/test_stack_reorder.py new file mode 100644 index 0000000000..d3336ce699 --- /dev/null +++ b/tests/unit/compiler/venom/test_stack_reorder.py @@ -0,0 +1,30 @@ +from vyper.venom.context import IRContext +from vyper.venom.venom_to_assembly import VenomCompiler + +from vyper.venom import generate_assembly_experimental + +def test_stack_reorder(): + """ + Test to was created from the example in the + issue https://github.com/vyperlang/vyper/issues/4215 + this example should fail with original stack reorder + algorithm but succeed with new one + """ + ctx = IRContext() + fn = ctx.create_function("_global") + + bb = fn.get_basic_block() + var0 = bb.append_instruction("store", 1) + var1 = bb.append_instruction("store", 2) + var2 = bb.append_instruction("store", 3) + var3 = bb.append_instruction("store", 4) + var4 = bb.append_instruction("store", 5) + + bb.append_instruction("staticcall", var0, var1, var2, var3, var4, var3) + + ret_val = bb.append_instruction("add", var4, var4) + + bb.append_instruction("ret", ret_val) + + generate_assembly_experimental(ctx) + From 52777bcc7b5fe2064cc68435571b4c400d712fb0 Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 9 Sep 2024 11:32:06 +0200 Subject: [PATCH 6/8] stack reorder regression test (lint) --- tests/unit/compiler/venom/test_stack_reorder.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/unit/compiler/venom/test_stack_reorder.py b/tests/unit/compiler/venom/test_stack_reorder.py index d3336ce699..a9f505984e 100644 --- a/tests/unit/compiler/venom/test_stack_reorder.py +++ b/tests/unit/compiler/venom/test_stack_reorder.py @@ -1,11 +1,10 @@ +from vyper.venom import generate_assembly_experimental from vyper.venom.context import IRContext -from vyper.venom.venom_to_assembly import VenomCompiler -from vyper.venom import generate_assembly_experimental def test_stack_reorder(): """ - Test to was created from the example in the + Test to was created from the example in the issue https://github.com/vyperlang/vyper/issues/4215 this example should fail with original stack reorder algorithm but succeed with new one @@ -23,8 +22,7 @@ def test_stack_reorder(): bb.append_instruction("staticcall", var0, var1, var2, var3, var4, var3) ret_val = bb.append_instruction("add", var4, var4) - + bb.append_instruction("ret", ret_val) generate_assembly_experimental(ctx) - From 3cde8a54e256bc199f91d466eb1319a0793068cf Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 9 Sep 2024 11:35:49 +0200 Subject: [PATCH 7/8] stack reorder regression test (lint) --- vyper/venom/venom_to_assembly.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index 5b838074da..20003c0515 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -214,7 +214,7 @@ def _stack_reorder( # positions stores the positions of relevant operands # on stack for example operand %82 is on positions [0, 3] # this operand could ocure even more deeper in the stack - # but only those that are needed/relevant in calculation + # but only those that are needed/relevant in calculation # are considered positions: dict[IROperand, list[int]] = defaultdict(lambda: []) for op in stack_ops: From 76441a8fa4bab32bf08b0f8cf80230b9b9a18a52 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 30 Sep 2024 11:41:00 -0400 Subject: [PATCH 8/8] cleanup --- vyper/venom/venom_to_assembly.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vyper/venom/venom_to_assembly.py b/vyper/venom/venom_to_assembly.py index 202236fa18..9de75dab38 100644 --- a/vyper/venom/venom_to_assembly.py +++ b/vyper/venom/venom_to_assembly.py @@ -206,6 +206,8 @@ def _stack_reorder( stack = stack.copy() stack_ops_count = len(stack_ops) + if stack_ops_count == 0: + return 0 counts = Counter(stack_ops) @@ -214,7 +216,7 @@ def _stack_reorder( # this operand could ocure even more deeper in the stack # but only those that are needed/relevant in calculation # are considered - positions: dict[IROperand, list[int]] = defaultdict(lambda: []) + positions: dict[IROperand, list[int]] = defaultdict(list) for op in stack_ops: positions[op] = [] for i in range(counts[op]): @@ -255,9 +257,7 @@ def _stack_reorder( cost += self.swap(assembly, stack, final_stack_depth) - assert ( - len(stack_ops) == 0 or stack._stack[-len(stack_ops) :] == stack_ops - ), f"Wrong stack {stack} {stack_ops}" + assert stack._stack[-len(stack_ops) :] == stack_ops, (stack, stack_ops) return cost