From fded6146294731fcc2ec405cc0ec2ff1447ba144 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Thu, 26 Oct 2023 11:26:23 -0400 Subject: [PATCH] add review comments --- vyper/codegen/dfg.py | 5 +++++ vyper/compiler/utils.py | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/vyper/codegen/dfg.py b/vyper/codegen/dfg.py index 6e3837b3fc..c604982036 100644 --- a/vyper/codegen/dfg.py +++ b/vyper/codegen/dfg.py @@ -259,6 +259,7 @@ def _generate_evm_for_basicblock_r( label_counter = 0 +# REVIEW: would this be better as a class? def _generate_evm_for_instruction_r( ctx: IRFunction, assembly: list, inst: IRInstruction, stack_map: StackMap ) -> list[str]: @@ -266,8 +267,10 @@ def _generate_evm_for_instruction_r( for op in inst.get_output_operands(): for target in ctx.dfg_inputs.get(op.value, []): + # REVIEW: what does this line do? if target.parent != inst.parent: continue + # REVIEW: what does this line do? if target.fen != inst.fen: continue assembly = _generate_evm_for_instruction_r(ctx, assembly, target, stack_map) @@ -298,6 +301,8 @@ def _generate_evm_for_instruction_r( if opcode == "select": # REVIEW: maybe call this 'phi' ret = inst.get_output_operands()[0] inputs = inst.get_input_operands() + # REVIEW: the special handling in get_depth_in for lists + # seems cursed, refactor depth = stack_map.get_depth_in(inputs) assert depth is not StackMap.NOT_IN_STACK, "Operand not in stack" to_be_replaced = stack_map.peek(depth) diff --git a/vyper/compiler/utils.py b/vyper/compiler/utils.py index fc86fdcf38..4056575260 100644 --- a/vyper/compiler/utils.py +++ b/vyper/compiler/utils.py @@ -49,9 +49,12 @@ def _expand_row(row): return result +# REVIEW: move this to vyper/ir/ or vyper/venom/ +# rename to StackModel class StackMap: NOT_IN_STACK = object() - stack_map: list[IRValueBase] + stack_map: list[IRValueBase] # REVIEW: rename to stack + # REVIEW: dead variable dependant_liveness: OrderedSet[IRValueBase] def __init__(self): @@ -93,12 +96,15 @@ def get_depth_in(self, op: IRValueBase | list[IRValueBase]) -> int: for i, stack_op in enumerate(self.stack_map[::-1]): if isinstance(stack_op, IRValueBase): + # REVIEW: handling literals this way seems a bit cursed, + # why not use IRLiteral, so it is always IRValueBase? if isinstance(op, str) and stack_op.value == op: return -i if isinstance(op, int) and stack_op.value == op: return -i if isinstance(op, IRValueBase) and stack_op.value == op.value: return -i + # REVIEW: this branch seems cursed elif isinstance(op, list) and stack_op in op: return -i