From 123049f6ac15dd1c42f055237af17d144c304232 Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 23 Dec 2024 09:15:24 +0100 Subject: [PATCH 1/5] loads dont flush all copies --- tests/unit/compiler/venom/test_memmerging.py | 26 ++++++++++++++++++++ vyper/venom/passes/memmerging.py | 24 +++++++++++++----- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/tests/unit/compiler/venom/test_memmerging.py b/tests/unit/compiler/venom/test_memmerging.py index b7a4d33805..fda0e70d22 100644 --- a/tests/unit/compiler/venom/test_memmerging.py +++ b/tests/unit/compiler/venom/test_memmerging.py @@ -17,6 +17,32 @@ def _check_pre_post(pre, post): def _check_no_change(pre): _check_pre_post(pre, pre) +def test_memmerging_tmp(): + if not version_check(begin="cancun"): + return + + pre = """ + main: + %1 = mload 352 + mstore 448, %1 + %2 = mload 416 + mstore 64, %2 + %3 = mload 448 ; barrier, flushes mload 416 from list of potential copies + mstore 96, %3 + stop + """ + + post = """ + main: + %1 = mload 352 + mstore 448, %1 + mcopy 64, 416, 64 + stop + """ + + _check_pre_post(pre, post) + + def test_memmerging(): """ diff --git a/vyper/venom/passes/memmerging.py b/vyper/venom/passes/memmerging.py index 41216e3041..2d0877e048 100644 --- a/vyper/venom/passes/memmerging.py +++ b/vyper/venom/passes/memmerging.py @@ -108,8 +108,8 @@ def run_pass(self): self.analyses_cache.invalidate_analysis(DFGAnalysis) self.analyses_cache.invalidate_analysis(LivenessAnalysis) - def _optimize_copy(self, bb: IRBasicBlock, copy_opcode: str, load_opcode: str): - for copy in self._copies: + def _optimize_copy(self, bb: IRBasicBlock, copies: list[_Copy], copy_opcode: str, load_opcode: str): + for copy in copies: copy.insts.sort(key=bb.instructions.index) if copy_opcode == "mcopy": @@ -156,8 +156,8 @@ def _optimize_copy(self, bb: IRBasicBlock, copy_opcode: str, load_opcode: str): bb.mark_for_removal(inst) - self._copies.clear() - self._loads.clear() + #self._copies.clear() + #self._loads.clear() def _write_after_write_hazard(self, new_copy: _Copy) -> bool: for copy in self._copies: @@ -217,7 +217,19 @@ def _handle_bb( self._copies = [] def _barrier(): - self._optimize_copy(bb, copy_opcode, load_opcode) + self._optimize_copy(bb, self._copies, copy_opcode, load_opcode) + self._copies.clear() + self._loads.clear() + + def _load_barrier(interval: _Interval): + copies = [c for c in self._copies if c.overwrites(interval)] + self._optimize_copy(bb, copies, copy_opcode, load_opcode) + self._copies = [c for c in self._copies if not c.overwrites(interval)] + for c in copies: + for inst in c.insts: + if inst.opcode == load_opcode: + assert isinstance(inst.output, IRVariable) + del self._loads[inst.output] # copy in necessary because there is a possibility # of insertion in optimizations @@ -232,7 +244,7 @@ def _barrier(): # we will read from this memory so we need to put barier if not allow_dst_overlaps_src and self._overwrites(read_interval): - _barrier() + _load_barrier(read_interval) assert inst.output is not None self._loads[inst.output] = src_op.value From 350750e840fafa6626d7cd52682e7b902ba1a645 Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 23 Dec 2024 11:34:26 +0100 Subject: [PATCH 2/5] write hazards flushed only needed copies --- tests/unit/compiler/venom/test_memmerging.py | 19 ++++++++--- vyper/venom/passes/memmerging.py | 35 ++++++++++++-------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/tests/unit/compiler/venom/test_memmerging.py b/tests/unit/compiler/venom/test_memmerging.py index fda0e70d22..f2640ec784 100644 --- a/tests/unit/compiler/venom/test_memmerging.py +++ b/tests/unit/compiler/venom/test_memmerging.py @@ -17,6 +17,7 @@ def _check_pre_post(pre, post): def _check_no_change(pre): _check_pre_post(pre, pre) + def test_memmerging_tmp(): if not version_check(begin="cancun"): return @@ -43,7 +44,6 @@ def test_memmerging_tmp(): _check_pre_post(pre, post) - def test_memmerging(): """ Basic memory merge test @@ -649,11 +649,20 @@ def test_memmerging_write_after_write(): %3 = mload 32 %4 = mload 132 mstore 1000, %1 - mstore 1000, %2 ; BARRIER + mstore 1000, %2 ; partial BARRIER mstore 1032, %4 mstore 1032, %3 ; BARRIER """ - _check_no_change(pre) + + post = """ + _global: + %1 = mload 0 + %3 = mload 32 + mstore 1000, %1 + mcopy 1000, 100, 64 + mstore 1032, %3 ; BARRIER + """ + _check_pre_post(pre, post) def test_memmerging_write_after_write_mstore_and_mcopy(): @@ -667,9 +676,9 @@ def test_memmerging_write_after_write_mstore_and_mcopy(): pre = """ _global: %1 = mload 0 - %2 = mload 132 + %2 = mload 32 mstore 1000, %1 - mcopy 1000, 100, 16 ; write barrier + mcopy 1000, 100, 64 ; write barrier mstore 1032, %2 mcopy 1016, 116, 64 stop diff --git a/vyper/venom/passes/memmerging.py b/vyper/venom/passes/memmerging.py index 2d0877e048..5eaaa6d1a7 100644 --- a/vyper/venom/passes/memmerging.py +++ b/vyper/venom/passes/memmerging.py @@ -108,7 +108,9 @@ def run_pass(self): self.analyses_cache.invalidate_analysis(DFGAnalysis) self.analyses_cache.invalidate_analysis(LivenessAnalysis) - def _optimize_copy(self, bb: IRBasicBlock, copies: list[_Copy], copy_opcode: str, load_opcode: str): + def _optimize_copy( + self, bb: IRBasicBlock, copies: list[_Copy], copy_opcode: str, load_opcode: str + ): for copy in copies: copy.insts.sort(key=bb.instructions.index) @@ -156,10 +158,11 @@ def _optimize_copy(self, bb: IRBasicBlock, copies: list[_Copy], copy_opcode: str bb.mark_for_removal(inst) - #self._copies.clear() - #self._loads.clear() + # self._copies.clear() + # self._loads.clear() - def _write_after_write_hazard(self, new_copy: _Copy) -> bool: + def _write_after_write_hazard(self, new_copy: _Copy) -> list[_Copy]: + res = [] for copy in self._copies: # note, these are the same: # - new_copy.overwrites(copy.dst_interval()) @@ -167,8 +170,9 @@ def _write_after_write_hazard(self, new_copy: _Copy) -> bool: if new_copy.overwrites(copy.dst_interval()) and not ( copy.can_merge(new_copy) or new_copy.can_merge(copy) ): - return True - return False + res.append(copy) + + return res def _read_after_write_hazard(self, new_copy: _Copy) -> bool: new_copies = self._copies + [new_copy] @@ -223,9 +227,12 @@ def _barrier(): def _load_barrier(interval: _Interval): copies = [c for c in self._copies if c.overwrites(interval)] + _barrier_for(copies) + + def _barrier_for(copies: list[_Copy]): self._optimize_copy(bb, copies, copy_opcode, load_opcode) - self._copies = [c for c in self._copies if not c.overwrites(interval)] for c in copies: + self._copies.remove(c) for inst in c.insts: if inst.opcode == load_opcode: assert isinstance(inst.output, IRVariable) @@ -265,8 +272,9 @@ def _load_barrier(interval: _Interval): assert load_inst is not None # help mypy n_copy = _Copy(dst.value, src_ptr, 32, [inst, load_inst]) - if self._write_after_write_hazard(n_copy): - _barrier() + write_hazards = self._write_after_write_hazard(n_copy) + if len(write_hazards) > 0: + _barrier_for(write_hazards) # no continue needed, we have not invalidated the loads dict # check if the new copy does not overwrites existing data @@ -285,8 +293,9 @@ def _load_barrier(interval: _Interval): length, src, dst = inst.operands n_copy = _Copy(dst.value, src.value, length.value, [inst]) - if self._write_after_write_hazard(n_copy): - _barrier() + write_hazards = self._write_after_write_hazard(n_copy) + if len(write_hazards) > 0: + _barrier_for(write_hazards) # check if the new copy does not overwrites existing data if not allow_dst_overlaps_src and self._read_after_write_hazard(n_copy): _barrier() @@ -338,7 +347,7 @@ def _barrier(): _barrier() continue n_copy = _Copy.memzero(dst.value, 32, [inst]) - assert not self._write_after_write_hazard(n_copy) + assert len(self._write_after_write_hazard(n_copy)) == 0 self._add_copy(n_copy) elif inst.opcode == "calldatacopy": length, var, dst = inst.operands @@ -354,7 +363,7 @@ def _barrier(): _barrier() continue n_copy = _Copy.memzero(dst.value, length.value, [inst]) - assert not self._write_after_write_hazard(n_copy) + assert len(self._write_after_write_hazard(n_copy)) == 0 self._add_copy(n_copy) elif _volatile_memory(inst): _barrier() From 8f7f5b8077431e97df354c8e50b7d9e35547e85e Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 23 Dec 2024 14:00:24 +0100 Subject: [PATCH 3/5] all hazards should only flush needed copies --- vyper/venom/passes/memmerging.py | 47 +++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/vyper/venom/passes/memmerging.py b/vyper/venom/passes/memmerging.py index 5eaaa6d1a7..161a87516e 100644 --- a/vyper/venom/passes/memmerging.py +++ b/vyper/venom/passes/memmerging.py @@ -174,20 +174,25 @@ def _write_after_write_hazard(self, new_copy: _Copy) -> list[_Copy]: return res - def _read_after_write_hazard(self, new_copy: _Copy) -> bool: - new_copies = self._copies + [new_copy] + def _read_after_write_hazard(self, new_copy: _Copy) -> list[_Copy]: + # new_copies = self._copies + [new_copy] + + res = [copy for copy in self._copies if new_copy.overwrites(copy.src_interval())] + [ + copy for copy in self._copies if copy.overwrites(new_copy.src_interval()) + ] + return res # new copy would overwrite memory that # needs to be read to optimize copy - if any(new_copy.overwrites(copy.src_interval()) for copy in new_copies): - return True + # if any(new_copy.overwrites(copy.src_interval()) for copy in new_copies): + # return True # existing copies would overwrite memory that the # new copy would need - if self._overwrites(new_copy.src_interval()): - return True + # if self._overwrites(new_copy.src_interval()): + # return True - return False + # return False def _find_insertion_point(self, new_copy: _Copy): return bisect_left(self._copies, new_copy.dst, key=lambda c: c.dst) @@ -232,11 +237,13 @@ def _load_barrier(interval: _Interval): def _barrier_for(copies: list[_Copy]): self._optimize_copy(bb, copies, copy_opcode, load_opcode) for c in copies: - self._copies.remove(c) + if c in self._copies: + self._copies.remove(c) for inst in c.insts: if inst.opcode == load_opcode: assert isinstance(inst.output, IRVariable) - del self._loads[inst.output] + if inst.output in self._loads: + del self._loads[inst.output] # copy in necessary because there is a possibility # of insertion in optimizations @@ -278,11 +285,15 @@ def _barrier_for(copies: list[_Copy]): # no continue needed, we have not invalidated the loads dict # check if the new copy does not overwrites existing data - if not allow_dst_overlaps_src and self._read_after_write_hazard(n_copy): - _barrier() - # this continue is necessary because we have invalidated - # the _loads dict, so src_ptr is no longer valid. - continue + if not allow_dst_overlaps_src: + read_hazards = self._read_after_write_hazard(n_copy) + if len(read_hazards) > 0: + _barrier_for(read_hazards) + # this continue is necessary because we have invalidated + # the _loads dict, so src_ptr is no longer valid. + continue + if n_copy.overwrites_self_src(): + continue self._add_copy(n_copy) elif inst.opcode == copy_opcode: @@ -297,8 +308,12 @@ def _barrier_for(copies: list[_Copy]): if len(write_hazards) > 0: _barrier_for(write_hazards) # check if the new copy does not overwrites existing data - if not allow_dst_overlaps_src and self._read_after_write_hazard(n_copy): - _barrier() + if not allow_dst_overlaps_src: + read_hazards = self._read_after_write_hazard(n_copy) + if len(read_hazards) > 0: + _barrier_for(read_hazards) + if n_copy.overwrites_self_src(): + continue self._add_copy(n_copy) elif _volatile_memory(inst): From 179836c8522560e872b8685366bfe2f11e342032 Mon Sep 17 00:00:00 2001 From: Hodan Date: Mon, 23 Dec 2024 14:02:29 +0100 Subject: [PATCH 4/5] removed commented code --- vyper/venom/passes/memmerging.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/vyper/venom/passes/memmerging.py b/vyper/venom/passes/memmerging.py index 161a87516e..d2c9e93613 100644 --- a/vyper/venom/passes/memmerging.py +++ b/vyper/venom/passes/memmerging.py @@ -182,18 +182,6 @@ def _read_after_write_hazard(self, new_copy: _Copy) -> list[_Copy]: ] return res - # new copy would overwrite memory that - # needs to be read to optimize copy - # if any(new_copy.overwrites(copy.src_interval()) for copy in new_copies): - # return True - - # existing copies would overwrite memory that the - # new copy would need - # if self._overwrites(new_copy.src_interval()): - # return True - - # return False - def _find_insertion_point(self, new_copy: _Copy): return bisect_left(self._copies, new_copy.dst, key=lambda c: c.dst) From 1e324b53b7bce3a057f2b4479b0521626c347000 Mon Sep 17 00:00:00 2001 From: Hodan Date: Tue, 31 Dec 2024 08:36:31 +0100 Subject: [PATCH 5/5] lint --- tests/unit/compiler/venom/test_memmerging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/compiler/venom/test_memmerging.py b/tests/unit/compiler/venom/test_memmerging.py index 0e42020daf..8beb50574c 100644 --- a/tests/unit/compiler/venom/test_memmerging.py +++ b/tests/unit/compiler/venom/test_memmerging.py @@ -43,7 +43,7 @@ def test_memmerging_tmp(): _check_pre_post(pre, post) - + # for parametrizing tests LOAD_COPY = [("dload", "dloadbytes"), ("calldataload", "calldatacopy")]