Fix an output_to_verilog bug where inlined arithmetic expressions might unintentionaly truncate.

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,20 +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 for Outputs, Inputs, Consts, or
-        Registers, because those declarations are handled separately by
-        ``_to_verilog_header``.
-
-        Temporary Verilog wires are never declared for memory writes, because writes
-        generate no output.
-
-        Otherwise, temporary Verilog wires are declared for:
+        Temporary Verilog wires are declared (``needs_declaration``) for:
 
         1. Named ``Gates``.
 
@@ -963,14 +958,44 @@ 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.
+
+        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``.
         """
-        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 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]:
         """Return the sanitized version of ``name`` and a Verilog comment with the
@@ -1063,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)
 
@@ -1081,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)
@@ -1285,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):
@@ -1485,6 +1510,7 @@ def output_to_verilog(
 
         >>> import pyrtl
         >>> pyrtl.reset_working_block()
+        >>> pyrtl.wire._reset_wire_indexers()
 
     Example::
 
@@ -1503,9 +1529,13 @@ def output_to_verilog(
             input[2:0] a;
             input[2:0] b;
             output[3:0] sum;
+        <BLANKLINE>
+            // Temporaries
+            wire[3:0] tmp0;
         <BLANKLINE>
             // 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

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 = """\