fix[venom]: add make_ssa pass after algebraic optimizations (#4292)

This commit adds a `MakeSSA` pass after algebraic optimisations. Makes
`StoreElimination` pass skip `phi` instructions and adds a test case
that would fail without this step. The `MakeSSA` pass here results in
smaller code so we are keeping it in for now.

Resolves GH issue #4288

---------

Co-authored-by: Charles Cooper <[email protected]>

tests/unit/compiler/venom/test_algebraic_optimizer.py

@@ -1,5 +1,6 @@
 import pytest
 
+import vyper
 from vyper.venom.analysis import IRAnalysesCache
 from vyper.venom.basicblock import IRBasicBlock, IRLabel
 from vyper.venom.context import IRContext
@@ -176,3 +177,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"])

vyper/venom/__init__.py

@@ -55,6 +55,13 @@ 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()
 

vyper/venom/analysis/cfg.py

@@ -32,7 +32,10 @@ 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(DominatorTreeAnalysis)
         self.analyses_cache.invalidate_analysis(LivenessAnalysis)
+
+        # be conservative - assume cfg invalidation invalidates dfg
+        self.analyses_cache.invalidate_analysis(DFGAnalysis)

vyper/venom/passes/sccp/sccp.py

@@ -332,19 +332,18 @@ 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")
 
     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]

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: