Repository: amaranth-lang/amaranth
Pull request #1397 (2024-06-15 00:20:11)

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


Patch:
diff --git a/amaranth/hdl/_cd.py b/amaranth/hdl/_cd.py
index 51a26e7..b4a8b2b 100644
--- a/amaranth/hdl/_cd.py
+++ b/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 @@ class ClockDomain:
             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 @@ class ClockDomain:
 
         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
diff --git a/amaranth/hdl/_ir.py b/amaranth/hdl/_ir.py
index f24cd2e..c750c54 100644
--- a/amaranth/hdl/_ir.py
+++ b/amaranth/hdl/_ir.py
@@ -82,7 +82,6 @@ class Fragment:
         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 @@ class Fragment:
     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 @@ class Fragment:
                 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 @@ class Fragment:
         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()
diff --git a/amaranth/hdl/_xfrm.py b/amaranth/hdl/_xfrm.py
index 92d0705..5d4d95c 100644
--- a/amaranth/hdl/_xfrm.py
+++ b/amaranth/hdl/_xfrm.py
@@ -464,10 +464,7 @@ class DomainCollector(ValueVisitor, StatementVisitor):
 
         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 @@ class DomainRenamer(FragmentTransformer, ValueTransformer, StatementTransformer)
 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 @@ class DomainLowerer(FragmentTransformer, ValueTransformer, StatementTransformer)
 
     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)
 
 
diff --git a/amaranth/lib/cdc.py b/amaranth/lib/cdc.py
index 1afcc76..776c1bc 100644
--- a/amaranth/lib/cdc.py
+++ b/amaranth/lib/cdc.py
@@ -173,7 +173,7 @@ class AsyncFFSynchronizer(Elaboratable):
                                       .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):
diff --git a/docs/changes.rst b/docs/changes.rst
index 22ca652..5731eef 100644
--- a/docs/changes.rst
+++ b/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


Test patch:
diff --git a/tests/test_hdl_cd.py b/tests/test_hdl_cd.py
index 5a9fa73..b262550 100644
--- a/tests/test_hdl_cd.py
+++ b/tests/test_hdl_cd.py
@@ -9,7 +9,6 @@ class ClockDomainTestCase(FHDLTestCase):
         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 @@ class ClockDomainTestCase(FHDLTestCase):
         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 @@ class ClockDomainTestCase(FHDLTestCase):
         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)
\ No newline at end of file
diff --git a/tests/test_hdl_ir.py b/tests/test_hdl_ir.py
index 5b6a682..060e67e 100644
--- a/tests/test_hdl_ir.py
+++ b/tests/test_hdl_ir.py
@@ -456,99 +456,6 @@ class FragmentPortsTestCase(FHDLTestCase):
 
 
 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 @@ class FragmentDomainsTestCase(FHDLTestCase):
                     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()