From 4517cdb875cf0a9de544db2b988c8b385cc88b81 Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Fri, 11 Oct 2024 19:52:58 +0300 Subject: [PATCH 1/9] make SSA after algebraic --- vyper/venom/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/vyper/venom/__init__.py b/vyper/venom/__init__.py index a5f51b787d..5fb5f25eff 100644 --- a/vyper/venom/__init__.py +++ b/vyper/venom/__init__.py @@ -53,6 +53,7 @@ def _run_passes(fn: IRFunction, optimize: OptimizationLevel) -> None: StoreElimination(ac, fn).run_pass() SimplifyCFGPass(ac, fn).run_pass() AlgebraicOptimizationPass(ac, fn).run_pass() + MakeSSA(ac, fn).run_pass() BranchOptimizationPass(ac, fn).run_pass() RemoveUnusedVariablesPass(ac, fn).run_pass() From f1f2b4fe31ae91f961604bd3cd37a46291c127bf Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Fri, 11 Oct 2024 20:05:21 +0300 Subject: [PATCH 2/9] add test case --- .../venom/test_algebraic_optimizer.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/unit/compiler/venom/test_algebraic_optimizer.py b/tests/unit/compiler/venom/test_algebraic_optimizer.py index b5d55efbdc..cd0a9e71f2 100644 --- a/tests/unit/compiler/venom/test_algebraic_optimizer.py +++ b/tests/unit/compiler/venom/test_algebraic_optimizer.py @@ -1,5 +1,6 @@ import pytest +import vyper from vyper.venom.analysis.analysis import IRAnalysesCache from vyper.venom.basicblock import IRBasicBlock, IRLabel from vyper.venom.context import IRContext @@ -178,3 +179,29 @@ def test_offsets(): offset_count += 1 assert offset_count == 3 + + +# Test the case of https://github.com/vyperlang/vyper/issues/4288 +def test_ssa_after_algebraic_optimization(): + code = """ +@internal +def _do_math(x: uint256) -> uint256: + value: uint256 = x + result: uint256 = 0 + + if (x >> 128 != 0): + x >>= 128 + if (x >> 64 != 0): + x >>= 64 + + if 1 < value: + result = 1 + + return result + +@external +def run() -> uint256: + return self._do_math(10) + """ + + vyper.compile_code(code, output_formats=["bytecode"]) From b7609d39b8069024e7ff5dc56f5520367e361cf1 Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Mon, 14 Oct 2024 14:50:06 +0300 Subject: [PATCH 3/9] remove makessa --- vyper/venom/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/venom/__init__.py b/vyper/venom/__init__.py index 783214ed96..eb378125e6 100644 --- a/vyper/venom/__init__.py +++ b/vyper/venom/__init__.py @@ -55,7 +55,7 @@ def _run_passes(fn: IRFunction, optimize: OptimizationLevel) -> None: StoreElimination(ac, fn).run_pass() SimplifyCFGPass(ac, fn).run_pass() AlgebraicOptimizationPass(ac, fn).run_pass() - MakeSSA(ac, fn).run_pass() + BranchOptimizationPass(ac, fn).run_pass() RemoveUnusedVariablesPass(ac, fn).run_pass() From cf9f4959e627bd054a488966d6c50a6ffe361a4d Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Mon, 14 Oct 2024 14:50:43 +0300 Subject: [PATCH 4/9] update store elimination to exclude all phis --- vyper/venom/passes/store_elimination.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/vyper/venom/passes/store_elimination.py b/vyper/venom/passes/store_elimination.py index 0ecd324e26..559205adc8 100644 --- a/vyper/venom/passes/store_elimination.py +++ b/vyper/venom/passes/store_elimination.py @@ -27,16 +27,17 @@ def run_pass(self): self.analyses_cache.invalidate_analysis(LivenessAnalysis) self.analyses_cache.invalidate_analysis(DFGAnalysis) - def _process_store(self, dfg, inst, var, new_var): + def _process_store(self, dfg, inst, var: IRVariable, new_var: IRVariable): """ Process store instruction. If the variable is only used by a load instruction, forward the variable to the load instruction. """ - uses = dfg.get_uses(var) + if any([inst.opcode == "phi" for inst in dfg.get_uses(new_var)]): + return + uses = dfg.get_uses(var) if any([inst.opcode == "phi" for inst in uses]): return - for use_inst in uses: for i, operand in enumerate(use_inst.operands): if operand == var: From 54d8a020df6bd2fe58153e21c6280a59da4903b2 Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Mon, 14 Oct 2024 15:36:02 +0300 Subject: [PATCH 5/9] running makessa after algebraic it currenly produces smaller code --- vyper/venom/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/venom/__init__.py b/vyper/venom/__init__.py index eb378125e6..783214ed96 100644 --- a/vyper/venom/__init__.py +++ b/vyper/venom/__init__.py @@ -55,7 +55,7 @@ def _run_passes(fn: IRFunction, optimize: OptimizationLevel) -> None: StoreElimination(ac, fn).run_pass() SimplifyCFGPass(ac, fn).run_pass() AlgebraicOptimizationPass(ac, fn).run_pass() - + MakeSSA(ac, fn).run_pass() BranchOptimizationPass(ac, fn).run_pass() RemoveUnusedVariablesPass(ac, fn).run_pass() From c27070f3e8661a9e8fbb26c9e48e94b5b00e0b0e Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Mon, 14 Oct 2024 15:38:46 +0300 Subject: [PATCH 6/9] fix sscp phi sorting and cleanup duplicate phis --- vyper/venom/passes/sccp/sccp.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/vyper/venom/passes/sccp/sccp.py b/vyper/venom/passes/sccp/sccp.py index 7966863081..9094115ecf 100644 --- a/vyper/venom/passes/sccp/sccp.py +++ b/vyper/venom/passes/sccp/sccp.py @@ -332,19 +332,35 @@ def _replace_constants(self, inst: IRInstruction): def _fix_phi_nodes(self): # fix basic blocks whose cfg in was changed # maybe this should really be done in _visit_phi - needs_sort = False for bb in self.fn.get_basic_blocks(): cfg_in_labels = OrderedSet(in_bb.label for in_bb in bb.cfg_in) + needs_sort = False for inst in bb.instructions: if inst.opcode != "phi": break needs_sort |= self._fix_phi_inst(inst, cfg_in_labels) - # move phi instructions to the top of the block - if needs_sort: - bb.instructions.sort(key=lambda inst: inst.opcode != "phi") + # move phi instructions to the top of the block + if needs_sort: + bb.instructions.sort(key=lambda inst: inst.opcode != "phi") + + # Cleanup duplicate phis + phis = dict[int, IRInstruction]() + phi_inst = [inst for inst in bb.instructions if inst.opcode == "phi"] + for inst in phi_inst: + op_hash = hash((op for op in inst.operands)) + if op_hash not in phis.keys(): + phis[op_hash] = inst + continue + + for use_inst in self._get_uses(inst.output): + for i, operand in enumerate(use_inst.operands): + if operand == inst.output: + use_inst.operands[i] = phis[op_hash].output + + bb.remove_instruction(inst) def _fix_phi_inst(self, inst: IRInstruction, cfg_in_labels: OrderedSet): operands = [op for label, op in inst.phi_operands if label in cfg_in_labels] From 5234a1a5eafb7b6b0276fd50db633c20be5d856c Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Mon, 14 Oct 2024 16:03:05 +0300 Subject: [PATCH 7/9] remove phi cleanup --- vyper/venom/analysis/cfg.py | 3 ++- vyper/venom/passes/sccp/sccp.py | 17 ----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/vyper/venom/analysis/cfg.py b/vyper/venom/analysis/cfg.py index e4f130bc18..ab405685a0 100644 --- a/vyper/venom/analysis/cfg.py +++ b/vyper/venom/analysis/cfg.py @@ -32,7 +32,8 @@ def analyze(self) -> None: in_bb.add_cfg_out(bb) def invalidate(self): - from vyper.venom.analysis import DominatorTreeAnalysis, LivenessAnalysis + from vyper.venom.analysis import DFGAnalysis, DominatorTreeAnalysis, LivenessAnalysis + self.analyses_cache.invalidate_analysis(DFGAnalysis) self.analyses_cache.invalidate_analysis(DominatorTreeAnalysis) self.analyses_cache.invalidate_analysis(LivenessAnalysis) diff --git a/vyper/venom/passes/sccp/sccp.py b/vyper/venom/passes/sccp/sccp.py index 9094115ecf..19d373f81a 100644 --- a/vyper/venom/passes/sccp/sccp.py +++ b/vyper/venom/passes/sccp/sccp.py @@ -332,7 +332,6 @@ def _replace_constants(self, inst: IRInstruction): def _fix_phi_nodes(self): # fix basic blocks whose cfg in was changed # maybe this should really be done in _visit_phi - for bb in self.fn.get_basic_blocks(): cfg_in_labels = OrderedSet(in_bb.label for in_bb in bb.cfg_in) @@ -346,22 +345,6 @@ def _fix_phi_nodes(self): if needs_sort: bb.instructions.sort(key=lambda inst: inst.opcode != "phi") - # Cleanup duplicate phis - phis = dict[int, IRInstruction]() - phi_inst = [inst for inst in bb.instructions if inst.opcode == "phi"] - for inst in phi_inst: - op_hash = hash((op for op in inst.operands)) - if op_hash not in phis.keys(): - phis[op_hash] = inst - continue - - for use_inst in self._get_uses(inst.output): - for i, operand in enumerate(use_inst.operands): - if operand == inst.output: - use_inst.operands[i] = phis[op_hash].output - - bb.remove_instruction(inst) - def _fix_phi_inst(self, inst: IRInstruction, cfg_in_labels: OrderedSet): operands = [op for label, op in inst.phi_operands if label in cfg_in_labels] From cbcacd3d2285d1433f5114a9e947b6160f30d914 Mon Sep 17 00:00:00 2001 From: Harry Kalogirou Date: Wed, 16 Oct 2024 10:37:32 +0300 Subject: [PATCH 8/9] add note --- vyper/venom/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vyper/venom/__init__.py b/vyper/venom/__init__.py index 783214ed96..bf3115b4dd 100644 --- a/vyper/venom/__init__.py +++ b/vyper/venom/__init__.py @@ -55,6 +55,12 @@ def _run_passes(fn: IRFunction, optimize: OptimizationLevel) -> None: StoreElimination(ac, fn).run_pass() SimplifyCFGPass(ac, fn).run_pass() AlgebraicOptimizationPass(ac, fn).run_pass() + # NOTE: MakeSSA is after algebraic optimization it currently produces + # smaller code by adding some redundant phi nodes. This is not a + # problem for us, but we need to be aware of it, and should be + # removed when the dft pass is fixed to produce the smallest code + # without making the code generation more expensive by running + # MakeSSA again. MakeSSA(ac, fn).run_pass() BranchOptimizationPass(ac, fn).run_pass() RemoveUnusedVariablesPass(ac, fn).run_pass() From 674b203d19f3ca3fd2b45b9f8ea8f2155e91c5cf Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Wed, 16 Oct 2024 12:05:01 -0400 Subject: [PATCH 9/9] add a note --- vyper/venom/analysis/cfg.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/vyper/venom/analysis/cfg.py b/vyper/venom/analysis/cfg.py index ab405685a0..90b18b353c 100644 --- a/vyper/venom/analysis/cfg.py +++ b/vyper/venom/analysis/cfg.py @@ -34,6 +34,8 @@ def analyze(self) -> None: def invalidate(self): from vyper.venom.analysis import DFGAnalysis, DominatorTreeAnalysis, LivenessAnalysis - self.analyses_cache.invalidate_analysis(DFGAnalysis) self.analyses_cache.invalidate_analysis(DominatorTreeAnalysis) self.analyses_cache.invalidate_analysis(LivenessAnalysis) + + # be conservative - assume cfg invalidation invalidates dfg + self.analyses_cache.invalidate_analysis(DFGAnalysis)