Implement RFC 59: Get rid of upwards propagation of clock domains.

Author: wanda-phi

amaranth/hdl/_cd.py

@@ -1,3 +1,5 @@
+import warnings
+
 from .. import tracer
 from ._ast import Signal
 
@@ -27,9 +29,6 @@ class ClockDomain:
         If ``True``, the domain uses an asynchronous reset, and registers within this domain
         are initialized to their reset state when reset level changes. Otherwise, registers
         are initialized to reset state at the next clock cycle when reset is asserted.
-    local : bool
-        If ``True``, the domain will propagate only downwards in the design hierarchy. Otherwise,
-        the domain will propagate everywhere.
 
     Attributes
     ----------
@@ -48,7 +47,7 @@ def _name_for(domain_name, signal_name):
             return f"{domain_name}_{signal_name}"
 
     def __init__(self, name=None, *, clk_edge="pos", reset_less=False, async_reset=False,
-                 local=False):
+                 local=None):
         if name is None:
             try:
                 name = tracer.get_var_name()
@@ -75,7 +74,17 @@ def __init__(self, name=None, *, clk_edge="pos", reset_less=False, async_reset=F
 
         self.async_reset = async_reset
 
-        self.local = local
+        # TODO(amaranth-0.7): remove
+        if local is not None:
+            warnings.warn(f"`local` is deprecated and has no effect",
+                          DeprecationWarning, stacklevel=2)
+
+    @property
+    def local(self):
+        # TODO(amaranth-0.7): remove
+        warnings.warn(f"`local` is deprecated and has no effect",
+                      DeprecationWarning, stacklevel=2)
+        return True
 
     def rename(self, new_name):
         self.name = new_name

amaranth/hdl/_ir.py

@@ -82,7 +82,6 @@ def __init__(self, *, src_loc=None):
         self.generated = OrderedDict()
         self.src_loc = src_loc
         self.origins = None
-        self.domains_propagated_up = {}
         self.domain_renames = {}
 
     def add_domains(self, *domains):
@@ -127,67 +126,6 @@ def find_generated(self, *path):
     def elaborate(self, platform):
         return self
 
-    def _propagate_domains_up(self, hierarchy=("top",)):
-        from ._xfrm import DomainRenamer
-
-        domain_subfrags = defaultdict(set)
-
-        # For each domain defined by a subfragment, determine which subfragments define it.
-        for i, (subfrag, name, src_loc) in enumerate(self.subfragments):
-            # First, recurse into subfragments and let them propagate domains up as well.
-            hier_name = name
-            if hier_name is None:
-                hier_name = f"<unnamed #{i}>"
-            subfrag._propagate_domains_up(hierarchy + (hier_name,))
-
-            # Second, classify subfragments by domains they define.
-            for domain_name, domain in subfrag.domains.items():
-                if domain.local:
-                    continue
-                domain_subfrags[domain_name].add((subfrag, name, src_loc, i))
-
-        # For each domain defined by more than one subfragment, rename the domain in each
-        # of the subfragments such that they no longer conflict.
-        for domain_name, subfrags in domain_subfrags.items():
-            if len(subfrags) == 1:
-                continue
-
-            names = [n for f, n, s, i in subfrags]
-            if not all(names):
-                names = sorted(f"<unnamed #{i}>" if n is None else f"'{n}'"
-                               for f, n, s, i in subfrags)
-                raise _cd.DomainError(
-                    "Domain '{}' is defined by subfragments {} of fragment '{}'; it is necessary "
-                    "to either rename subfragment domains explicitly, or give names to subfragments"
-                    .format(domain_name, ", ".join(names), ".".join(hierarchy)))
-
-            if len(names) != len(set(names)):
-                names = sorted(f"#{i}" for f, n, s, i in subfrags)
-                raise _cd.DomainError(
-                    "Domain '{}' is defined by subfragments {} of fragment '{}', some of which "
-                    "have identical names; it is necessary to either rename subfragment domains "
-                    "explicitly, or give distinct names to subfragments"
-                    .format(domain_name, ", ".join(names), ".".join(hierarchy)))
-
-            for subfrag, name, src_loc, i in subfrags:
-                domain_name_map = {domain_name: f"{name}_{domain_name}"}
-                self.subfragments[i] = (DomainRenamer(domain_name_map)(subfrag), name, src_loc)
-
-        # Finally, collect the (now unique) subfragment domains, and merge them into our domains.
-        for i, (subfrag, name, src_loc) in enumerate(self.subfragments):
-            hier_name = name
-            if hier_name is None:
-                hier_name = f"<unnamed #{i}>"
-            for domain_name, domain in subfrag.domains.items():
-                if domain.local:
-                    continue
-                self.add_domains(domain)
-                if domain_name in subfrag.domains_propagated_up:
-                    _used_in, defined_in = subfrag.domains_propagated_up[domain_name]
-                else:
-                    defined_in = (*hierarchy, hier_name)
-                self.domains_propagated_up[domain_name] = (hierarchy, defined_in)
-
     def _propagate_domains_down(self, hierarchy=("top",)):
         # For each domain defined in this fragment, ensure it also exists in all subfragments.
         for i, (subfrag, name, src_loc) in enumerate(self.subfragments):
@@ -196,13 +134,8 @@ def _propagate_domains_down(self, hierarchy=("top",)):
                 hier_name = f"<unnamed #{i}>"
 
             for domain in self.iter_domains():
-                if domain in subfrag.domains:
-                    assert self.domains[domain] is subfrag.domains[domain]
-                else:
+                if domain not in subfrag.domains:
                     subfrag.add_domains(self.domains[domain])
-                    if domain in self.domains_propagated_up:
-                        _used_in, defined_in = self.domains_propagated_up[domain]
-                        subfrag.domains_propagated_up[domain] = (hierarchy + (hier_name,)), defined_in
 
             subfrag._propagate_domains_down(hierarchy + (hier_name,))
 
@@ -237,7 +170,6 @@ def _create_missing_domains(self, missing_domain, *, platform=None):
         return new_domains
 
     def _propagate_domains(self, missing_domain, *, platform=None):
-        self._propagate_domains_up()
         self._propagate_domains_down()
         new_domains = self._create_missing_domains(missing_domain, platform=platform)
         self._propagate_domains_down()

amaranth/hdl/_xfrm.py

@@ -464,10 +464,7 @@ def on_fragment(self, fragment):
 
         old_local_domains, self._local_domains = self._local_domains, set(self._local_domains)
         for domain_name, domain in fragment.domains.items():
-            if domain.local:
-                self._local_domains.add(domain_name)
-            else:
-                self.defined_domains.add(domain_name)
+            self._local_domains.add(domain_name)
 
         for domain_name, statements in fragment.statements.items():
             self._add_used_domain(domain_name)
@@ -549,26 +546,11 @@ def on_fragment(self, fragment):
 class DomainLowerer(FragmentTransformer, ValueTransformer, StatementTransformer):
     def __init__(self, domains=None):
         self.domains = domains
-        self.domains_propagated_up = {}
-
-    def _warn_on_propagation_up(self, domain, src_loc):
-        if domain in self.domains_propagated_up:
-            used_in, defined_in = self.domains_propagated_up[domain]
-            common_prefix = []
-            for u, d in zip(used_in, defined_in):
-                if u == d:
-                    common_prefix.append(u)
-            warnings.warn_explicit(f"Domain '{domain}' is used in '{'.'.join(used_in)}', but "
-                                   f"defined in '{'.'.join(defined_in)}', which will not be "
-                                   f"supported in Amaranth 0.6; define the domain in "
-                                   f"'{'.'.join(common_prefix)}' or one of its parents",
-                                   DeprecationWarning, filename=src_loc[0], lineno=src_loc[1])
 
     def _resolve(self, domain, context):
         if domain not in self.domains:
             raise DomainError("Signal {!r} refers to nonexistent domain '{}'"
                               .format(context, domain))
-        self._warn_on_propagation_up(domain, context.src_loc)
         return self.domains[domain]
 
     def replace_value_src_loc(self, value, new_value):
@@ -590,14 +572,6 @@ def on_ResetSignal(self, value):
 
     def on_fragment(self, fragment):
         self.domains = fragment.domains
-        self.domains_propagated_up = fragment.domains_propagated_up
-        for domain, statements in fragment.statements.items():
-            self._warn_on_propagation_up(domain, statements[0].src_loc)
-        if isinstance(fragment, MemoryInstance):
-            for port in fragment._read_ports:
-                self._warn_on_propagation_up(port._domain, fragment.src_loc)
-            for port in fragment._write_ports:
-                self._warn_on_propagation_up(port._domain, fragment.src_loc)
         return super().on_fragment(fragment)
 
 

amaranth/lib/cdc.py

@@ -173,7 +173,7 @@ def elaborate(self, platform):
                                       .format(type(platform).__name__))
 
         m = Module()
-        m.domains += ClockDomain("async_ff", async_reset=True, local=True)
+        m.domains += ClockDomain("async_ff", async_reset=True)
         flops = [Signal(1, name=f"stage{index}", init=1)
                  for index in range(self._stages)]
         for i, o in zip((0, *flops), flops):

docs/changes.rst

@@ -29,11 +29,13 @@ Language changes
 .. currentmodule:: amaranth.hdl
 
 * Changed: overriding :meth:`ValueCastable.from_bits` is now mandatory. (`RFC 51`_)
+* Deprecated: the :py:`local=` argument to :class:`ClockDomain`. (`RFC 59`_)
 * Removed: (deprecated in 0.4) :class:`Record`.
 * Removed: (deprecated in 0.5) :class:`Memory` (`RFC 45`_)
 * Removed: (deprecated in 0.5) public submodules of :mod:`amaranth.hdl`.
 * Removed: (deprecated in 0.5) :meth:`Value.implies`.
-* Removed: (deprecated in 0.5) :meth:`Const.width`, :meth:`Const.signed`, :meth:`Signal.width`, :meth:`Signal.signed`
+* Removed: (deprecated in 0.5) :meth:`Const.width`, :meth:`Const.signed`, :meth:`Signal.width`, :meth:`Signal.signed`.
+* Removed: (deprecated in 0.5) upwards propagation of clock domains. (`RFC 59`_)
 
 
 Standard library changes

tests/test_hdl_cd.py

@@ -9,7 +9,6 @@ def test_name(self):
         self.assertEqual(sync.name, "sync")
         self.assertEqual(sync.clk.name, "clk")
         self.assertEqual(sync.rst.name, "rst")
-        self.assertEqual(sync.local, False)
         pix = ClockDomain()
         self.assertEqual(pix.name, "pix")
         self.assertEqual(pix.clk.name, "pix_clk")
@@ -21,8 +20,6 @@ def test_name(self):
         with self.assertRaisesRegex(ValueError,
                 r"^Clock domain name must be specified explicitly$"):
             ClockDomain()
-        cd_reset = ClockDomain(local=True)
-        self.assertEqual(cd_reset.local, True)
 
     def test_edge(self):
         sync = ClockDomain()
@@ -77,3 +74,11 @@ def test_wrong_name_comb(self):
         with self.assertRaisesRegex(ValueError,
                 r"^Domain 'comb' may not be clocked$"):
             comb = ClockDomain()
+
+    def test_local(self):
+        with self.assertWarnsRegex(DeprecationWarning,
+                r"^`local` is deprecated and has no effect$"):
+            sync = ClockDomain(local=False)
+        with self.assertWarnsRegex(DeprecationWarning,
+                r"^`local` is deprecated and has no effect$"):
+            self.assertTrue(sync.local)

tests/test_hdl_ir.py

@@ -456,99 +456,6 @@ def test_port_not_iterable(self):
 
 
 class FragmentDomainsTestCase(FHDLTestCase):
-    def test_propagate_up(self):
-        cd = ClockDomain()
-
-        f1 = Fragment()
-        f2 = Fragment()
-        f1.add_subfragment(f2)
-        f2.add_domains(cd)
-
-        f1._propagate_domains_up()
-        self.assertEqual(f1.domains, {"cd": cd})
-
-    def test_propagate_up_local(self):
-        cd = ClockDomain(local=True)
-
-        f1 = Fragment()
-        f2 = Fragment()
-        f1.add_subfragment(f2)
-        f2.add_domains(cd)
-
-        f1._propagate_domains_up()
-        self.assertEqual(f1.domains, {})
-
-    def test_domain_conflict(self):
-        cda = ClockDomain("sync")
-        cdb = ClockDomain("sync")
-
-        fa = Fragment()
-        fa.add_domains(cda)
-        fb = Fragment()
-        fb.add_domains(cdb)
-        f = Fragment()
-        f.add_subfragment(fa, "a")
-        f.add_subfragment(fb, "b")
-
-        f._propagate_domains_up()
-        self.assertEqual(f.domains, {"a_sync": cda, "b_sync": cdb})
-        (fa, _, _), (fb, _, _) = f.subfragments
-        self.assertEqual(fa.domains, {"a_sync": cda})
-        self.assertEqual(fb.domains, {"b_sync": cdb})
-
-    def test_domain_conflict_anon(self):
-        cda = ClockDomain("sync")
-        cdb = ClockDomain("sync")
-
-        fa = Fragment()
-        fa.add_domains(cda)
-        fb = Fragment()
-        fb.add_domains(cdb)
-        f = Fragment()
-        f.add_subfragment(fa, "a")
-        f.add_subfragment(fb)
-
-        with self.assertRaisesRegex(DomainError,
-                (r"^Domain 'sync' is defined by subfragments 'a', <unnamed #1> of fragment "
-                    r"'top'; it is necessary to either rename subfragment domains explicitly, "
-                    r"or give names to subfragments$")):
-            f._propagate_domains_up()
-
-    def test_domain_conflict_name(self):
-        cda = ClockDomain("sync")
-        cdb = ClockDomain("sync")
-
-        fa = Fragment()
-        fa.add_domains(cda)
-        fb = Fragment()
-        fb.add_domains(cdb)
-        f = Fragment()
-        f.add_subfragment(fa, "x")
-        f.add_subfragment(fb, "x")
-
-        with self.assertRaisesRegex(DomainError,
-                (r"^Domain 'sync' is defined by subfragments #0, #1 of fragment 'top', some "
-                    r"of which have identical names; it is necessary to either rename subfragment "
-                    r"domains explicitly, or give distinct names to subfragments$")):
-            f._propagate_domains_up()
-
-    def test_domain_conflict_rename_drivers(self):
-        cda = ClockDomain("sync")
-        cdb = ClockDomain("sync")
-
-        fa = Fragment()
-        fa.add_domains(cda)
-        fb = Fragment()
-        fb.add_domains(cdb)
-        fb.add_statements("comb", ResetSignal("sync").eq(1))
-        f = Fragment()
-        f.add_subfragment(fa, "a")
-        f.add_subfragment(fb, "b")
-
-        f._propagate_domains_up()
-        fb_new, _, _ = f.subfragments[1]
-        self.assertRepr(fb_new.statements["comb"], "((eq (rst b_sync) (const 1'd1)))")
-
     def test_domain_conflict_rename_drivers_before_creating_missing(self):
         cda = ClockDomain("sync")
         cdb = ClockDomain("sync")
@@ -673,29 +580,15 @@ def test_propagate_create_missing_fragment_wrong(self):
                     r"domain 'sync' \(defines 'foo'\)\.$")):
             f1._propagate_domains(missing_domain=lambda name: f2)
 
-    def test_propagate_up_use(self):
-        a = Signal()
-        m = Module()
-        m1 = Module()
-        m2 = Module()
-        m.submodules.m1 = m1
-        m.submodules.m2 = m2
-        m1.d.test += a.eq(1)
-        m2.domains.test = ClockDomain()
-        with self.assertWarnsRegex(DeprecationWarning,
-                r"^Domain 'test' is used in 'top.m1', but defined in 'top.m2', which will not be "
-                r"supported in Amaranth 0.6; define the domain in 'top' or one of its parents$"):
-            Fragment.get(m, None).prepare()
-
 
 class FragmentHierarchyConflictTestCase(FHDLTestCase):
     def test_no_conflict_local_domains(self):
         f1 = Fragment()
-        cd1 = ClockDomain("d", local=True)
+        cd1 = ClockDomain("d")
         f1.add_domains(cd1)
         f1.add_statements("comb", ClockSignal("d").eq(1))
         f2 = Fragment()
-        cd2 = ClockDomain("d", local=True)
+        cd2 = ClockDomain("d")
         f2.add_domains(cd2)
         f2.add_statements("comb", ClockSignal("d").eq(1))
         f3 = Fragment()