Update output_to_verilog to inline temporary wires, using GateGraph: (#471)

* Update `output_to_verilog` to inline temporary wires, using `GateGraph`:

- Gates with user-specified names are never inlined.
- Unnamed constant Gates are always inlined, unless they are args to a bit-slice Gate.
- Unnamed non-constant Gates are inlined if:
  - The Gate has one user, AND
  - The Gate is not a MemBlock read, AND
  - The Gate is not an arg to a bit-slice Gate.

This significantly reduces the amount of generated Verilog code.

Existing bugs fixed:

- `output_verilog_testbench` should not re-initialize RomBlocks.
- FastSimulation was not updating `init_menvalues` correctly. 
- Specify bitwidths for Verilog initial register and memory values. They were
  previously unsized constants, which are implicitly 32-bit signed, which could
  cause surprises.

Add tests for these bugs.

Also:

- Improve Verilog identifier naming. Try replacing non-word characters with underscores before giving up and generating a new random name.
- When we mangle names, add a comment with the un-mangled name.
- Use sanitized MemBlock and RomBlock names instead of `mem_{memid}` names.
- Refactor `output_to_verilog` and `output_verilog_testbench` to share more code.
- Update `output_verilog_testbench` to use `GateGraph`.
- Define `GateGraph.__iter__` to make it easier to iterate over all Gates.
- Delete some disabled tests in `test_aes.py`.

Author: fdxmw

docs/blocks.rst

@@ -52,5 +52,5 @@ GateGraphs
 
 .. autoclass:: pyrtl.GateGraph
     :members:
-    :special-members: __init__, __str__
+    :special-members: __init__, __iter__, __str__
 

pyrtl/__init__.py

@@ -103,7 +103,6 @@
 from .importexport import (
     input_from_verilog,
     output_to_verilog,
-    OutputToVerilog,
     output_verilog_testbench,
     input_from_blif,
     output_to_firrtl,
@@ -242,7 +241,6 @@
     # importexport
     "input_from_verilog",
     "output_to_verilog",
-    "OutputToVerilog",
     "output_verilog_testbench",
     "input_from_blif",
     "output_to_firrtl",

pyrtl/compilesim.py

@@ -141,10 +141,9 @@ def __init__(
                 rval = self.default_value
             self._regmap[r] = rval
 
-        # Passing the dictionary objects themselves since they aren't updated anywhere.
-        # If that's ever not the case, will need to pass in deep copies of them like
-        # done for the normal Simulation so we retain the initial values that had.
-        self.tracer._set_initial_values(default_value, self._regmap, self._memmap)
+        self.tracer._set_initial_values(
+            default_value, register_value_map, memory_value_map
+        )
 
         self._create_dll()
         self._initialize_mems()

pyrtl/core.py

@@ -1193,58 +1193,108 @@ def next_index(self):
 
 
 class _NameSanitizer(_NameIndexer):
-    """Sanitizes the names so that names can be used in places that don't allow for
-    arbitrary names while not mangling valid names.
+    """Sanitizes names so they can be used in contexts that don't allow arbitrary names.
 
-    Put the values you want to validate into make_valid_string the first time you want
-    to sanitize a particular string (or before the first time), and retrieve from the
-    _NameSanitizer through indexing directly thereafter eg: sani["__&sfhs"] for
-    retrieval after the first time
+    For example, ``a.b`` is a valid ``WireVector`` name, but ``a.b`` is not a valid name
+    for a Python variable. If we want to generate Python code for ``a.b`` (like
+    ``FastSimulation``), the name must be sanitized.
+
+    Sanitization first attempts to replace non-word characters (anything that's not
+    alphanumeric or an underscore) with an underscore. If that didn't work, we try
+    appending a unique integer value. If that still doesn't work, we generate an
+    entirely new name consisting of ``internal_prefix`` followed by a unique integer
+    value.
+
+    ``make_valid_string`` must be called once to generate the sanitized version of a
+    name.
+
+    .. doctest only::
+
+        >>> import pyrtl
+
+    After ``make_valid_string`` has been called, the sanitized name can be retrieved
+    with ``__getitem__`` any number of times. For example::
+
+        >>> sanitizer = pyrtl.core._NameSanitizer(pyrtl.core._py_regex)
+
+        >>> sanitizer.make_valid_string("foo.bar")
+        'foo_bar'
+        >>> sanitizer["foo.bar"]
+        'foo_bar'
+        >>> sanitizer["foo.bar"]
+        'foo_bar'
+
+        >>> sanitizer.make_valid_string("foo_bar")
+        'foo_bar0'
     """
 
     def __init__(
         self,
         identifier_regex_str,
         internal_prefix="_sani_temp",
-        map_valid_vals=True,
         extra_checks=lambda _string: True,
-        allow_duplicates=False,
     ):
         if identifier_regex_str[-1] != "$":
             identifier_regex_str += "$"
         self.identifier = re.compile(identifier_regex_str)
+        # Map from un-sanitized name to sanitized name.
         self.val_map = {}
-        self.map_valid = map_valid_vals
+        # Set of all generated sanitized names.
+        self.sanitized_names = set()
         self.extra_checks = extra_checks
-        self.allow_dups = allow_duplicates
         super().__init__(internal_prefix)
 
-    def __getitem__(self, item):
-        """Get a value from the sanitizer"""
-        if not self.map_valid and self.is_valid_str(item):
-            return item
-        return self.val_map[item]
+    def __getitem__(self, name: str) -> str:
+        """Return the sanitized name for an un-sanitized name that was generated by
+        ``make_valid_string``.
+        """
+        return self.val_map[name]
 
-    def is_valid_str(self, string):
+    def is_valid_str(self, string: str) -> bool:
+        """Return ``True`` iff ``string`` matches ``identifier_regex_str`` and satisfies
+        ``extra_checks``.
+        """
         return self.identifier.match(string) and self.extra_checks(string)
 
-    def make_valid_string(self, string=""):
-        """Inputting a value for the first time."""
-        if not self.is_valid_str(string):
-            if string in self.val_map and not self.allow_dups:
-                msg = f"Value {string} has already been given to the sanitizer"
-                raise IndexError(msg)
-            internal_name = super().make_valid_string()
-            self.val_map[string] = internal_name
-            return internal_name
-        if self.map_valid:
-            self.val_map[string] = string
-        return string
+    def make_valid_string(self, string: str = "") -> str:
+        """Generate a sanitized name from an un-sanitized name."""
+        if string in self.val_map:
+            msg = f"Value {string} has already been given to the sanitizer"
+            raise IndexError(msg)
+
+        def is_usable(name: str) -> bool:
+            """Return ``True`` iff ``name`` can be used as a sanitized name.
+
+            A sanitized name is usable if it ``is_valid_str``, and isn't already in use.
+            """
+            return self.is_valid_str(name) and name not in self.sanitized_names
+
+        internal_name = string
+        if not is_usable(internal_name):
+            # Try replacing non-word characters with ``_``.
+            internal_name = re.sub(r"\W", "_", string)
+
+            if not is_usable(internal_name):
+                # If that didn't work, try appending the next ``internal_index``.
+                internal_name = f"{internal_name}{self.next_index()}"
+
+                if not is_usable(internal_name):
+                    # If that didn't work, generate an entirely new name starting with
+                    # ``internal_prefix``.
+                    internal_name = super().make_valid_string()
+
+                    if not is_usable(internal_name):
+                        msg = f"Could not generate a usable sanitized name for {string}"
+                        raise PyrtlError(msg)
+
+        self.val_map[string] = internal_name
+        self.sanitized_names.add(internal_name)
+        return internal_name
 
 
 class _PythonSanitizer(_NameSanitizer):
     """Name Sanitizer specifically built for Python identifers."""
 
-    def __init__(self, internal_prefix="_sani_temp", map_valid_vals=True):
-        super().__init__(_py_regex, internal_prefix, map_valid_vals)
+    def __init__(self, internal_prefix="_sani_temp"):
+        super().__init__(_py_regex, internal_prefix)
         self.extra_checks = lambda s: not keyword.iskeyword(s)

pyrtl/gate_graph.py

@@ -712,6 +712,9 @@ class GateGraph:
     gates: set[Gate]
     """A :class:`set` of all :class:`Gates<Gate>` in the ``GateGraph``.
 
+    Similar to :attr:`~GateGraph.__iter__`, except that ``gates`` is a :class:`set`
+    rather than an :class:`~collections.abc.Iterable`.
+
     .. doctest only::
 
         >>> import pyrtl
@@ -1114,3 +1117,28 @@ def __str__(self) -> str:
             self.gates, key=lambda gate: gate.name if gate.name else "~~~"
         )
         return "\n".join([str(gate) for gate in sorted_gates])
+
+    def __iter__(self):
+        """Iterate over each gate in the :class:`GateGraph`.
+
+        Similar to :attr:`~GateGraph.gates`, except that ``__iter__`` returns an
+        :class:`~collections.abc.Iterable` rather than a :class:`set`.
+
+        .. doctest only::
+
+            >>> import pyrtl
+            >>> pyrtl.reset_working_block()
+
+        Example::
+
+            >>> a = pyrtl.Input(name="a", bitwidth=2)
+            >>> b = pyrtl.Input(name="b", bitwidth=2)
+            >>> sum = a + b
+            >>> sum.name = "sum"
+
+            >>> gate_graph = pyrtl.GateGraph()
+
+            >>> sorted(gate.name for gate in gate_graph)
+            ['a', 'b', 'sum']
+        """
+        return iter(self.gates)