From b53b2b9d7a3d70fc59a05e45ef80812cea54bac4 Mon Sep 17 00:00:00 2001 From: Jeremy Lau <30300826+fdxmw@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:10:54 -0700 Subject: [PATCH 1/2] Fix an `output_to_verilog` bug where inlined arithmetic expressions might unintentionaly truncate. In Verilog, the expression `a + b` in the example below has bitwidth 6: input[5:0] a; input[5:0] b; output[7:0] o; assign o = {1'd1, a + b}; This is a major difference from PyRTL, where `a + b` has bitwidth 7. Assignment to `o` in this example may trigger a warning, because the concatenation expression has bitwidth 7 (`1'd1` has bitwidth 1, `a + b` has bitwidth 6), while `o` has bitwidth 8. Loss of the addition's carry bit can be prevented by explicitly assigning the result to a wider-bitwidth wire: wire[6:0] tmp; assign tmp = a + b; assign o = {1'd1, tmp}; This commit prevents `pyrtl.output_to_verilog` from inlining arithmetic expressions (`+`, `-`, `*`) to avoid triggering this implicit truncation in Verilog. We can't safely inline these arithmetic expressions because the carry bits may be needed. We could analyze the `GateGraph` and check if the carry bits are actually used. For now we keep things simple and never inline arithmetic expressions. --- pyrtl/importexport.py | 42 ++++++++++++++++++++++------ tests/test_importexport.py | 56 ++++++++++++++++++++++++++++++-------- 2 files changed, 79 insertions(+), 19 deletions(-) diff --git a/pyrtl/importexport.py b/pyrtl/importexport.py index d5f7a19a..dc36c33b 100644 --- a/pyrtl/importexport.py +++ b/pyrtl/importexport.py @@ -945,14 +945,16 @@ def _should_declare_wire(self, gate: Gate) -> bool: This function determines which temporary Gates will be declared in Verilog. Any temporary gate without a corresponding Verilog declaration will be inlined. - Temporary Verilog wires are never declared for Outputs, Inputs, Consts, or - Registers, because those declarations are handled separately by - ``_to_verilog_header``. + Temporary Verilog wires are never declared (``excluded``) for:: - Temporary Verilog wires are never declared for memory writes, because writes - generate no output. - Otherwise, temporary Verilog wires are declared for: + 1. Outputs (``is_output``), Inputs (``I``), Consts (``C``), or Registers + (``r``), because those declarations are handled separately by + ``_to_verilog_header``. + + 2. Memory writes (``@``), because writes generate no output. + + Otherwise, temporary Verilog wires are declared (``needs_declaration``) for: 1. Named ``Gates``. @@ -963,14 +965,33 @@ def _should_declare_wire(self, gate: Gate) -> bool: 4. ``Gates`` that are ``args`` for a ``s`` bit-selection ``Gate``, because Verilog's bit-selection operator ``[]`` only works on wires and registers, not arbitrary expressions. + + 5. Arithmetic operations (``+``, ``-``, ``*``), because Verilog arithmetic + operations implicitly truncate to input bitwidth, unless the arithmetic + expression is explicitly assigned to a wider-bitwidth wire. """ excluded = gate.is_output or gate.op in "ICr@" + is_named = gate.name and not gate.name.startswith("tmp") multiple_users = len(gate.dests) > 1 is_read = gate.op == "m" is_sliced = self._is_sliced(gate) - return not excluded and (is_named or multiple_users or is_read or is_sliced) + # In PyRTL, addition increases input bitwidth by 1, and multiplication doubles + # input bitwidth. But in Verilog, multiplication and addition truncate to the + # input bitwidth, *unless* the arithmetic expression is explicitly assigned to a + # wider-bitwidth wire. + # + # So we must declare wires for all arithmetic operations, to stop Verilog from + # truncating the carry bits. We could analyze the GateGraph to check if the + # carry bits are actually used, but we keep this simple for now. + is_arithmetic = gate.op in "+-*" + + needs_declaration = ( + is_named or multiple_users or is_read or is_sliced or is_arithmetic + ) + + return needs_declaration and not excluded def _name_and_comment(self, name: str, kind="") -> tuple[str, str]: """Return the sanitized version of ``name`` and a Verilog comment with the @@ -1485,6 +1506,7 @@ def output_to_verilog( >>> import pyrtl >>> pyrtl.reset_working_block() + >>> pyrtl.wire._reset_wire_indexers() Example:: @@ -1503,9 +1525,13 @@ def output_to_verilog( input[2:0] a; input[2:0] b; output[3:0] sum; + + // Temporaries + wire[3:0] tmp0; // Combinational logic - assign sum = (a + b); + assign sum = tmp0; + assign tmp0 = (a + b); endmodule :param dest_file: Open file where the Verilog output will be written. Defaults to diff --git a/tests/test_importexport.py b/tests/test_importexport.py index 2ee1ab7a..8a68629b 100644 --- a/tests/test_importexport.py +++ b/tests/test_importexport.py @@ -838,9 +838,11 @@ def test_blif_nor_gate_correct(self): wire[4:0] tmp54; wire[4:0] tmp58; wire[4:0] tmp60; + wire[5:0] tmp63; wire[6:0] tmp66; wire[4:0] tmp70; wire[3:0] tmp72; + wire[5:0] tmp75; wire[3:0] tmp76; wire[6:0] tmp79; @@ -859,9 +861,11 @@ def test_blif_nor_gate_correct(self): assign tmp54 = (r + {{3 {1'd0}}, 1'd1}); assign tmp58 = (r + {{3 {1'd0}}, 1'd1}); assign tmp60 = (a + r); - assign tmp66 = ((tmp60 + {{4 {1'd0}}, 1'd1}) - {{2 {1'd0}}, s}); + assign tmp63 = (tmp60 + {{4 {1'd0}}, 1'd1}); + assign tmp66 = (tmp63 - {{2 {1'd0}}, s}); assign tmp70 = (a - {{3 {1'd0}}, 1'd1}); - assign tmp79 = ((tmp60 + {(1'd0), tmp72}) + {{2 {1'd0}}, tmp76}); + assign tmp75 = (tmp60 + {(1'd0), tmp72}); + assign tmp79 = (tmp75 + {{2 {1'd0}}, tmp76}); // Register logic always @(posedge clk) begin @@ -1104,10 +1108,12 @@ def test_blif_nor_gate_correct(self): reg[3:0] r; // Temporaries + wire[4:0] tmp2; wire[4:0] tmp5; // Combinational logic - assign tmp5 = (rst ? {{4 {1'd0}}, 1'd0} : (r + {{3 {1'd0}}, 1'd1})); + assign tmp2 = (r + {{3 {1'd0}}, 1'd1}); + assign tmp5 = (rst ? {{4 {1'd0}}, 1'd0} : tmp2); // Register logic always @(posedge clk) begin @@ -1185,7 +1191,7 @@ def test_textual_consistency_small(self): buffer = io.StringIO() pyrtl.output_to_verilog(buffer, add_reset=False) - self.assertEqual(buffer.getvalue(), verilog_output_small) + self.assertEqual(verilog_output_small, buffer.getvalue()) def test_textual_consistency_large(self): # The following is a non-sensical program created to test that the Verilog that @@ -1216,7 +1222,7 @@ def test_textual_consistency_large(self): buffer = io.StringIO() pyrtl.output_to_verilog(buffer) - self.assertEqual(buffer.getvalue(), verilog_output_large) + self.assertEqual(verilog_output_large, buffer.getvalue()) def test_mems_with_no_writes(self): rdata = {0: 10, 1: 20, 2: 30, 3: 40, 4: 50, 5: 60} @@ -1233,7 +1239,7 @@ def test_mems_with_no_writes(self): buffer = io.StringIO() pyrtl.output_to_verilog(buffer) - self.assertEqual(buffer.getvalue(), verilog_output_mems_with_no_writes) + self.assertEqual(verilog_output_mems_with_no_writes, buffer.getvalue()) def check_counter_text(self, add_reset, expected): r = pyrtl.Register(bitwidth=4, reset_value=2) @@ -1243,7 +1249,7 @@ def check_counter_text(self, add_reset, expected): buffer = io.StringIO() pyrtl.output_to_verilog(buffer, add_reset) - self.assertEqual(buffer.getvalue(), expected) + self.assertEqual(expected, buffer.getvalue()) def test_textual_consistency_with_sync_reset(self): self.check_counter_text(True, verilog_output_counter_sync_reset) @@ -1273,7 +1279,7 @@ def test_existing_reset_wire_without_add_reset(self): r = pyrtl.Register(bitwidth=4, name="r") r.next <<= pyrtl.select(rst, 0, r + 1) pyrtl.output_to_verilog(buffer, add_reset=False) - self.assertEqual(buffer.getvalue(), verilog_custom_reset) + self.assertEqual(verilog_custom_reset, buffer.getvalue()) def test_register_reset_value(self): register0 = pyrtl.Register(name="register0", bitwidth=8, reset_value=0) @@ -1319,6 +1325,34 @@ def test_bit_slice_inputs(self): # bit-slice. self.assertTrue("assign tmp3 = c" in buffer.getvalue()) + def test_arithmetic_operations(self): + """Verify that wires are always declared for arithmetic operations. + + These wires must be declared in case the carry bits are needed. + """ + a = pyrtl.Input(name="a", bitwidth=6) + b = pyrtl.Input(name="b", bitwidth=6) + concat_sum = pyrtl.Output(name="concat_sum", bitwidth=8) + concat_diff = pyrtl.Output(name="concat_diff", bitwidth=8) + concat_prod = pyrtl.Output(name="concat_prod", bitwidth=13) + + # In PyRTL, `a + b` and `b - a` have bitwidth 7, and concatenating `1` results + # in bitwidth 8. In Verilog, `a + b` and `b - a` have bitwidth 6, *unless* the + # arithmetic expression is explicitly assigned to a wider-bitwidth wire. + concat_sum <<= pyrtl.concat(1, a + b) + concat_diff <<= pyrtl.concat(1, b - a) + # Similarly, `a * b` has bitwidth 12 in PyRTL, but bitwidth 6 in Verilog, + # *unless* the multiplication is explicitly assigned to a wider-bitwidth wire. + concat_prod <<= pyrtl.concat(1, a * b) + + buffer = io.StringIO() + pyrtl.output_to_verilog(buffer) + + print(buffer.getvalue()) + self.assertTrue("assign tmp0 = (a + b)" in buffer.getvalue()) + self.assertTrue("assign tmp2 = (b - a)" in buffer.getvalue()) + self.assertTrue("assign tmp4 = (a * b)" in buffer.getvalue()) + def test_custom_module_name(self): a, b = pyrtl.Input(1, "a"), pyrtl.Input(1, "b") out = pyrtl.Output(name="out") @@ -1651,7 +1685,7 @@ def test_verilog_testbench_existing_reset_wire_without_add_reset(self): buffer = io.StringIO() pyrtl.output_verilog_testbench(buffer, add_reset=False) - self.assertEqual(buffer.getvalue(), verilog_testbench_custom_reset) + self.assertEqual(verilog_testbench_custom_reset, buffer.getvalue()) def test_only_initialize_memblocks(self): """Test that RomBlocks are not re-initialized by the testbench.""" @@ -1765,7 +1799,7 @@ def test_textual_consistency_concats(self): buffer = io.StringIO() pyrtl.output_to_firrtl(buffer) - self.assertEqual(buffer.getvalue(), firrtl_output_concat_test) + self.assertEqual(firrtl_output_concat_test, buffer.getvalue()) def test_textual_consistency_selects(self): a = pyrtl.Const(0b101101001101) @@ -1775,7 +1809,7 @@ def test_textual_consistency_selects(self): buffer = io.StringIO() pyrtl.output_to_firrtl(buffer) - self.assertEqual(buffer.getvalue(), firrtl_output_select_test) + self.assertEqual(firrtl_output_select_test, buffer.getvalue()) iscas85_bench_c432 = """\ From 0c59ea086972e3b1aac07658d44aaa5c263fe9a4 Mon Sep 17 00:00:00 2001 From: Jeremy Lau <30300826+fdxmw@users.noreply.github.com> Date: Tue, 28 Jul 2026 09:46:53 -0700 Subject: [PATCH 2/2] Improve comments and documentation. --- pyrtl/importexport.py | 54 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/pyrtl/importexport.py b/pyrtl/importexport.py index dc36c33b..c32d51da 100644 --- a/pyrtl/importexport.py +++ b/pyrtl/importexport.py @@ -920,11 +920,12 @@ def _is_sliced(self, gate: Gate) -> bool: and any(dest.op == "s" for dest in gate.dests) ) - def _should_declare_const(self, gate: Gate) -> bool: - """Determine if we should declare a constant Verilog wire for ``gate``. + def _must_declare_const(self, gate: Gate) -> bool: + """Determine if we must declare a constant Verilog wire for ``gate``. This function determines which constant Gates will be declared in Verilog. Any - constant gate without a corresponding Verilog declaration will be inlined. + constant gate without a corresponding Verilog declaration will be inlined in + Verilog. Constant Verilog gates are declared for: @@ -939,22 +940,14 @@ def _should_declare_const(self, gate: Gate) -> bool: is_sliced = self._is_sliced(gate) return is_const and (is_named or is_sliced) - def _should_declare_wire(self, gate: Gate) -> bool: - """Determine if we should declare a temporary Verilog wire for ``gate``. + def _must_declare_wire(self, gate: Gate) -> bool: + """Determine if we must declare a temporary Verilog ``wire`` for ``gate``. - This function determines which temporary Gates will be declared in Verilog. Any - temporary gate without a corresponding Verilog declaration will be inlined. + When this function retuns ``True``, a Verilog ``wire`` will be declared for + ``gate``. When this function returns ``False``, ``gate`` will be inlined in + Verilog. - Temporary Verilog wires are never declared (``excluded``) for:: - - - 1. Outputs (``is_output``), Inputs (``I``), Consts (``C``), or Registers - (``r``), because those declarations are handled separately by - ``_to_verilog_header``. - - 2. Memory writes (``@``), because writes generate no output. - - Otherwise, temporary Verilog wires are declared (``needs_declaration``) for: + Temporary Verilog wires are declared (``needs_declaration``) for: 1. Named ``Gates``. @@ -969,9 +962,18 @@ def _should_declare_wire(self, gate: Gate) -> bool: 5. Arithmetic operations (``+``, ``-``, ``*``), because Verilog arithmetic operations implicitly truncate to input bitwidth, unless the arithmetic expression is explicitly assigned to a wider-bitwidth wire. - """ - excluded = gate.is_output or gate.op in "ICr@" + There are some exceptions. Temporary Verilog wires are never declared + (``excluded``) for: + + 1. Outputs (``is_output``), Inputs (``I``), Consts (``C``), or Registers + (``r``), because those declarations are handled separately by + ``_to_verilog_header``. + + 2. Memory writes (``@``), because writes generate no output. + + :return: ``True`` iff a Verilog ``wire`` must be declared for ``gate``. + """ is_named = gate.name and not gate.name.startswith("tmp") multiple_users = len(gate.dests) > 1 is_read = gate.op == "m" @@ -982,15 +984,17 @@ def _should_declare_wire(self, gate: Gate) -> bool: # input bitwidth, *unless* the arithmetic expression is explicitly assigned to a # wider-bitwidth wire. # - # So we must declare wires for all arithmetic operations, to stop Verilog from - # truncating the carry bits. We could analyze the GateGraph to check if the - # carry bits are actually used, but we keep this simple for now. + # So we must declare wider-bitwidth wires for all arithmetic operations, to + # prevent Verilog from truncating carry bits. We could analyze the GateGraph to + # check if the carry bits are actually used, but we keep this simple for now. is_arithmetic = gate.op in "+-*" needs_declaration = ( is_named or multiple_users or is_read or is_sliced or is_arithmetic ) + excluded = gate.is_output or gate.op in "ICr@" + return needs_declaration and not excluded def _name_and_comment(self, name: str, kind="") -> tuple[str, str]: @@ -1084,7 +1088,7 @@ def _to_verilog_header(self, file: TextIO, initialize_registers: bool): # Declare constants. const_gates = [] for const_gate in self._name_sorted(self.gate_graph.consts): - if self._should_declare_const(const_gate): + if self._must_declare_const(const_gate): const_gates.append(const_gate) self.declared_gates |= set(const_gates) @@ -1102,7 +1106,7 @@ def _to_verilog_header(self, file: TextIO, initialize_registers: bool): # Declare any needed temporary wires. temp_gates = [] for gate in self.gate_graph: - if self._should_declare_wire(gate): + if self._must_declare_wire(gate): temp_gates.append(gate) temp_gates = self._name_sorted(temp_gates) self.declared_gates |= set(temp_gates) @@ -1306,7 +1310,7 @@ def _to_verilog_memories(self, file: TextIO): ) print(" end", file=file) - # Find reads from ``memblock``. The ``read_gate`` should have been declared + # Find reads from ``memblock``. The ``read_gate`` must have been declared # by ``_to_verilog_header``. read_gates = [] for read_gate in self._name_sorted(self.gate_graph.mem_reads):