feat[venom]: make revert a bb terminator (#4529)

this commit makes `revert` a basic block terminator. previously
`revert` was left as a non-terminating instruction since there could
be the possibility of reordering. however, since it terminates, it is
cleaner to disallow any control flow after the `revert` instruction.

it also fixes a minor bug discovered in `ir_node_to_venom`, where the
translator was appending instructions after a `revert` instruction (it
would have been appending them to a dead block in any case).

Author: charles-cooper

tests/functional/venom/parser/test_parsing.py

@@ -55,7 +55,6 @@ def test_multi_bb_single_fn():
     has_callvalue_bb = IRBasicBlock(IRLabel("has_callvalue"), start_fn)
     start_fn.append_basic_block(has_callvalue_bb)
     has_callvalue_bb.append_instruction("revert", IRLiteral(0), IRLiteral(0))
-    has_callvalue_bb.append_instruction("stop")
 
     assert_ctx_eq(parsed_ctx, expected_ctx)
 
@@ -152,7 +151,6 @@ def test_multi_function():
 
     check_fn.append_basic_block(value_bb := IRBasicBlock(IRLabel("has_value"), check_fn))
     value_bb.append_instruction("revert", IRLiteral(0), IRLiteral(0))
-    value_bb.append_instruction("stop")
 
     assert_ctx_eq(parsed_ctx, expected_ctx)
 
@@ -215,7 +213,6 @@ def test_multi_function_and_data():
 
     check_fn.append_basic_block(value_bb := IRBasicBlock(IRLabel("has_value"), check_fn))
     value_bb.append_instruction("revert", IRLiteral(0), IRLiteral(0))
-    value_bb.append_instruction("stop")
 
     expected_ctx.data_segment = [
         DataSection(
@@ -313,7 +310,6 @@ def test_phis():
         %50 = 0
         %51 = 0
         revert %51, %50
-        stop
         ; (__main_entry)
     }  ; close function __main_entry
     """

vyper/venom/basicblock.py

@@ -8,7 +8,9 @@
 from vyper.utils import OrderedSet
 
 # instructions which can terminate a basic block
-BB_TERMINATORS = frozenset(["jmp", "djmp", "jnz", "ret", "return", "stop", "exit", "sink"])
+BB_TERMINATORS = frozenset(
+    ["jmp", "djmp", "jnz", "ret", "return", "revert", "stop", "exit", "sink"]
+)
 
 VOLATILE_INSTRUCTIONS = frozenset(
     [
@@ -602,11 +604,7 @@ def remove_instructions_after(self, instruction: IRInstruction) -> None:
 
     def ensure_well_formed(self):
         for inst in self.instructions:
-            assert inst.parent == self  # sanity
-            if inst.opcode == "revert":
-                self.remove_instructions_after(inst)
-                self.append_instruction("stop")  # TODO: make revert a bb terminator?
-                break
+            assert inst.parent == self  # sanity check
 
         def key(inst):
             if inst.opcode in ("phi", "param"):

vyper/venom/context.py

@@ -88,14 +88,6 @@ def get_next_variable(self) -> IRVariable:
     def get_last_variable(self) -> str:
         return f"%{self.last_variable}"
 
-    def chain_basic_blocks(self) -> None:
-        """
-        Chain basic blocks together. This is necessary for the IR to be valid, and is done after
-        the IR is generated.
-        """
-        for fn in self.functions.values():
-            fn.chain_basic_blocks()
-
     def append_data_section(self, name: IRLabel) -> None:
         self.data_segment.append(DataSection(name))
 

vyper/venom/function.py

@@ -209,28 +209,6 @@ def ast_source(self) -> Optional[IRnode]:
     def error_msg(self) -> Optional[str]:
         return self._error_msg_stack[-1] if len(self._error_msg_stack) > 0 else None
 
-    def chain_basic_blocks(self) -> None:
-        """
-        Chain basic blocks together. If a basic block is not terminated, jump to the next one.
-        Otherwise, append a stop instruction. This is necessary for the IR to be valid, and is
-        done after the IR is generated.
-        """
-        bbs = list(self.get_basic_blocks())
-        for i, bb in enumerate(bbs):
-            if bb.is_terminated:
-                continue
-
-            if i < len(bbs) - 1:
-                # TODO: revisit this. When contructor calls internal functions
-                # they are linked to the last ctor block. Should separate them
-                # before this so we don't have to handle this here
-                if bbs[i + 1].label.value.startswith("internal"):
-                    bb.append_instruction("stop")
-                else:
-                    bb.append_instruction("jmp", bbs[i + 1].label)
-            else:
-                bb.append_instruction("stop")
-
     def copy(self):
         new = IRFunction(self.name)
         for bb in self.get_basic_blocks():

vyper/venom/ir_node_to_venom.py

@@ -125,8 +125,6 @@ def ir_node_to_venom(ir: IRnode) -> IRContext:
 
     _convert_ir_bb(fn, ir, {})
 
-    ctx.chain_basic_blocks()
-
     for fn in ctx.functions.values():
         for bb in fn.get_basic_blocks():
             bb.ensure_well_formed()
@@ -418,14 +416,15 @@ def _convert_ir_bb(fn, ir, symbols):
         code = ir.args[2]
         _convert_ir_bb(fn, code, symbols)
     elif ir.value == "exit_to":
-        args = _convert_ir_bb_list(fn, ir.args[1:], symbols)
-        var_list = args
-        # TODO: only append return args if the function is external
-        _append_return_args(fn, *var_list)
         bb = fn.get_basic_block()
         if bb.is_terminated:
             bb = IRBasicBlock(ctx.get_next_label("exit_to"), fn)
             fn.append_basic_block(bb)
+
+        args = _convert_ir_bb_list(fn, ir.args[1:], symbols)
+        var_list = args
+        # TODO: only append return args if the function is external
+        _append_return_args(fn, *var_list)
         bb = fn.get_basic_block()
 
         label = IRLabel(ir.args[0].value)

vyper/venom/parser.py

@@ -88,15 +88,6 @@ def _set_last_label(ctx: IRContext):
                 ctx.last_label = max(int(label_head), ctx.last_label)
 
 
-def _ensure_terminated(bb):
-    # Since "revert" is not considered terminal explicitly check for it
-    # to ensure basic blocks are terminating
-    if not bb.is_terminated:
-        if any(inst.opcode == "revert" for inst in bb.instructions):
-            bb.append_instruction("stop")
-        # note: check_venom_ctx will raise error later if still not terminated.
-
-
 def _unescape(s: str):
     """
     Unescape the escaped string. This is the inverse of `IRLabel.__repr__()`.
@@ -136,8 +127,6 @@ def start(self, children) -> IRContext:
                     assert isinstance(instruction, IRInstruction)  # help mypy
                     bb.insert_instruction(instruction)
 
-                _ensure_terminated(bb)
-
             _set_last_var(fn)
         _set_last_label(ctx)
 

vyper/venom/passes/function_inliner.py

@@ -143,10 +143,6 @@ def _inline_call_site(self, func: IRFunction, call_site: IRInstruction) -> None:
                 elif inst.opcode == "ret":
                     inst.opcode = "jmp"
                     inst.operands = [call_site_return.label]
-                elif inst.opcode == "revert":
-                    bb.remove_instructions_after(inst)
-                    bb.append_instruction("stop")
-                    break
 
             for inst in bb.instructions:
                 if not inst.annotation: