From bf905ced9758be7f958b20035321a6f422a8c2f5 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Sat, 23 Mar 2024 11:33:29 -0500 Subject: [PATCH 1/2] hdl.ast: allow Signals to be privately named using `name=""` * Given a private name `$\d+` in RTLIL (as they are not named in the IR) * Not automatically added to VCD files (as they are not named in the IR) * Cannot be traced to a VCD (as they have no name to put in the file) * Cannot be used with an unnamed top-level port (as there is no name) --- amaranth/hdl/_ast.py | 13 ++++++++++--- amaranth/hdl/_ir.py | 4 +++- amaranth/sim/core.py | 11 +++++++++++ tests/test_hdl_ast.py | 4 ++++ tests/test_hdl_ir.py | 14 ++++++++++++-- tests/test_sim.py | 16 ++++++++++++++++ 6 files changed, 56 insertions(+), 6 deletions(-) diff --git a/amaranth/hdl/_ast.py b/amaranth/hdl/_ast.py index 1844880ef..551fcc388 100644 --- a/amaranth/hdl/_ast.py +++ b/amaranth/hdl/_ast.py @@ -1884,7 +1884,8 @@ class Signal(Value, DUID, metaclass=_SignalMeta): If not specified, ``shape`` defaults to 1-bit and non-signed. name : str Name hint for this signal. If ``None`` (default) the name is inferred from the variable - name this ``Signal`` is assigned to. + name this ``Signal`` is assigned to. If the empty string, then this ``Signal`` is treated + as private and is generally hidden from view. init : int or integral Enum Reset (synchronous) or default (combinatorial) value. When this ``Signal`` is assigned to in synchronous context and the corresponding clock @@ -1920,7 +1921,10 @@ def __init__(self, shape=None, *, name=None, init=None, reset=None, reset_less=F if name is not None and not isinstance(name, str): raise TypeError(f"Name must be a string, not {name!r}") - self.name = name or tracer.get_var_name(depth=2 + src_loc_at, default="$signal") + if name is None: + self.name = tracer.get_var_name(depth=2 + src_loc_at, default="$signal") + else: + self.name = name orig_shape = shape if shape is None: @@ -2102,7 +2106,10 @@ def _rhs_signals(self): return SignalSet((self,)) def __repr__(self): - return f"(sig {self.name})" + if self.name != "": + return f"(sig {self.name})" + else: + return "(sig)" @final diff --git a/amaranth/hdl/_ir.py b/amaranth/hdl/_ir.py index e02bae210..f3e9fc9ae 100644 --- a/amaranth/hdl/_ir.py +++ b/amaranth/hdl/_ir.py @@ -552,6 +552,8 @@ def _assign_port_names(self): assigned_names = {name for name, conn, dir in self.ports if name is not None} for name, conn, dir in self.ports: if name is None: + if conn.name == "": # Nothing to name this port! + raise TypeError("Signals with private names cannot be used in unnamed top-level ports") name = _add_name(assigned_names, conn.name) assigned_names.add(name) new_ports.append((name, conn, dir)) @@ -590,7 +592,7 @@ def _assign_names(self, fragment: Fragment, hierarchy: "tuple[str]"): frag_info.io_port_names[conn] = name for signal in frag_info.used_signals: - if signal not in frag_info.signal_names: + if signal not in frag_info.signal_names and signal.name != "": # Private name shouldn't be added. frag_info.signal_names[signal] = _add_name(frag_info.assigned_names, signal.name) for port in frag_info.used_io_ports: if port not in frag_info.io_port_names: diff --git a/amaranth/sim/core.py b/amaranth/sim/core.py index 67c4f1029..b54792e30 100644 --- a/amaranth/sim/core.py +++ b/amaranth/sim/core.py @@ -4,6 +4,7 @@ from .._utils import deprecated from ..hdl._cd import * from ..hdl._ir import * +from ..hdl._ast import Value, ValueLike from ._base import BaseEngine @@ -238,5 +239,15 @@ def write_vcd(self, vcd_file, gtkw_file=None, *, traces=(), fs_per_delta=0): file.close() raise ValueError("Cannot start writing waveforms after advancing simulation time") + for trace in traces: + if isinstance(trace, ValueLike): + trace_cast = Value.cast(trace) + for trace_signal in trace_cast._rhs_signals(): + if trace_signal.name == "": + if trace_signal is trace: + raise TypeError("Cannot trace signal with private name") + else: + raise TypeError(f"Cannot trace signal with private name (within {trace!r})") + return self._engine.write_vcd(vcd_file=vcd_file, gtkw_file=gtkw_file, traces=traces, fs_per_delta=fs_per_delta) diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py index d233b23f6..576aa90af 100644 --- a/tests/test_hdl_ast.py +++ b/tests/test_hdl_ast.py @@ -1180,6 +1180,8 @@ def test_name(self): self.assertEqual(s1.name, "s1") s2 = Signal(name="sig") self.assertEqual(s2.name, "sig") + s3 = Signal(name="") + self.assertEqual(s3.name, "") def test_init(self): s1 = Signal(4, init=0b111, reset_less=True) @@ -1294,6 +1296,8 @@ def test_attrs(self): def test_repr(self): s1 = Signal() self.assertEqual(repr(s1), "(sig s1)") + s2 = Signal(name="") + self.assertEqual(repr(s2), "(sig)") def test_like(self): s1 = Signal.like(Signal(4)) diff --git a/tests/test_hdl_ir.py b/tests/test_hdl_ir.py index f9683796c..133c32fd4 100644 --- a/tests/test_hdl_ir.py +++ b/tests/test_hdl_ir.py @@ -962,6 +962,7 @@ def test_assign_names_to_signals(self): o1 = Signal() o2 = Signal() o3 = Signal() + o4 = Signal(name="") i1 = Signal(name="i") f = Fragment() @@ -980,6 +981,7 @@ def test_assign_names_to_signals(self): "o1": (o1, PortDirection.Output), "o2": (o2, PortDirection.Output), "o3": (o3, PortDirection.Output), + "o4": (o4, PortDirection.Output), } design = f.prepare(ports) self.assertEqual(design.fragments[design.fragment].signal_names, SignalDict([ @@ -988,12 +990,20 @@ def test_assign_names_to_signals(self): (o1, "o1"), (o2, "o2"), (o3, "o3"), + # (o4, "o4"), # Signal has a private name. (cd_sync.clk, "clk"), - (cd_sync.rst, "rst$6"), + (cd_sync.rst, "rst$7"), (cd_sync_norst.clk, "sync_norst_clk"), - (i1, "i$7"), + (i1, "i$8"), ])) + def test_wrong_private_unnamed_toplevel_ports(self): + s = Signal(name="") + f = Fragment() + with self.assertRaisesRegex(TypeError, + r"^Signals with private names cannot be used in unnamed top-level ports$"): + Design(f, ports=((None, s, None),), hierarchy=("top",)) + def test_assign_names_to_fragments(self): f = Fragment() f.add_subfragment(a := Fragment()) diff --git a/tests/test_sim.py b/tests/test_sim.py index 978f8c090..9be42990e 100644 --- a/tests/test_sim.py +++ b/tests/test_sim.py @@ -14,6 +14,7 @@ from amaranth.hdl._ir import * from amaranth.sim import * from amaranth.lib.memory import Memory +from amaranth.lib.data import View, StructLayout from .utils import * from amaranth._utils import _ignore_deprecated @@ -1042,6 +1043,21 @@ def test_vcd_wrong_nonzero_time(self): with sim.write_vcd(f): pass + def test_vcd_private_signal(self): + sim = Simulator(Module()) + with self.assertRaisesRegex(TypeError, + r"^Cannot trace signal with private name$"): + with open(os.path.devnull, "w") as f: + with sim.write_vcd(f, traces=(Signal(name=""),)): + pass + + sim = Simulator(Module()) + with self.assertRaisesRegex(TypeError, + r"^Cannot trace signal with private name \(within \(cat \(sig x\) \(sig\)\)\)$"): + with open(os.path.devnull, "w") as f: + with sim.write_vcd(f, traces=(Cat(Signal(name="x"), Signal(name="")),)): + pass + def test_no_negated_boolean_warning(self): m = Module() a = Signal() From eb0fe16ec79eaa28a71c7ca8fe94ea881c58e2da Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Sat, 23 Mar 2024 11:44:44 -0500 Subject: [PATCH 2/2] hdl.dsl: use private names for FSM ongoing signals --- amaranth/hdl/_dsl.py | 6 +++--- tests/test_hdl_dsl.py | 23 +++++++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/amaranth/hdl/_dsl.py b/amaranth/hdl/_dsl.py index a74fd7cc3..2322a831c 100644 --- a/amaranth/hdl/_dsl.py +++ b/amaranth/hdl/_dsl.py @@ -167,7 +167,7 @@ def ongoing(self, name): if name not in self.encoding: self.encoding[name] = len(self.encoding) fsm_name = self._data["name"] - self._data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}") + self._data["ongoing"][name] = Signal(name="") return self._data["ongoing"][name] @@ -462,7 +462,7 @@ def State(self, name): if name not in fsm_data["encoding"]: fsm_name = fsm_data["name"] fsm_data["encoding"][name] = len(fsm_data["encoding"]) - fsm_data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}") + fsm_data["ongoing"][name] = Signal(name="") try: _outer_case, self._statements = self._statements, {} self._ctrl_context = None @@ -486,7 +486,7 @@ def next(self, name): if name not in ctrl_data["encoding"]: fsm_name = ctrl_data["name"] ctrl_data["encoding"][name] = len(ctrl_data["encoding"]) - ctrl_data["ongoing"][name] = Signal(name=f"{fsm_name}_ongoing_{name}") + ctrl_data["ongoing"][name] = Signal(name="") self._add_statement( assigns=[FSMNextStatement(ctrl_data, name)], domain=ctrl_data["domain"], diff --git a/tests/test_hdl_dsl.py b/tests/test_hdl_dsl.py index c3e8c023c..8048e5458 100644 --- a/tests/test_hdl_dsl.py +++ b/tests/test_hdl_dsl.py @@ -603,8 +603,8 @@ def test_FSM_basic(self): ) (case 1 ) ) - (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0))) - (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1))) + (eq (sig) (== (sig fsm_state) (const 1'd0))) + (eq (sig) (== (sig fsm_state) (const 1'd1))) ) """) self.assertRepr(frag.statements["sync"], """ @@ -627,8 +627,7 @@ def test_FSM_basic(self): "(sig a)": "comb", "(sig fsm_state)": "sync", "(sig b)": "sync", - "(sig fsm_ongoing_FIRST)": "comb", - "(sig fsm_ongoing_SECOND)": "comb", + "(sig)": "comb", }) fsm = frag.find_generated("fsm") self.assertIsInstance(fsm.state, Signal) @@ -659,8 +658,8 @@ def test_FSM_init(self): ) (case 1 ) ) - (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0))) - (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1))) + (eq (sig) (== (sig fsm_state) (const 1'd0))) + (eq (sig) (== (sig fsm_state) (const 1'd1))) ) """) self.assertRepr(frag.statements["sync"], """ @@ -697,8 +696,8 @@ def test_FSM_reset(self): ) (case 1 ) ) - (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd0))) - (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd1))) + (eq (sig) (== (sig fsm_state) (const 1'd0))) + (eq (sig) (== (sig fsm_state) (const 1'd1))) ) """) self.assertRepr(frag.statements["sync"], """ @@ -743,10 +742,10 @@ def test_FSM_ongoing(self): self.maxDiff = 10000 self.assertRepr(frag.statements["comb"], """ ( - (eq (sig b) (sig fsm_ongoing_SECOND)) - (eq (sig a) (sig fsm_ongoing_FIRST)) - (eq (sig fsm_ongoing_SECOND) (== (sig fsm_state) (const 1'd0))) - (eq (sig fsm_ongoing_FIRST) (== (sig fsm_state) (const 1'd1))) + (eq (sig b) (sig)) + (eq (sig a) (sig)) + (eq (sig) (== (sig fsm_state) (const 1'd0))) + (eq (sig) (== (sig fsm_state) (const 1'd1))) ) """)