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

Author: wanda-phi

amaranth/hdl/_ir.py

@@ -67,6 +67,7 @@ def __init__(self, *, src_loc=None):
         self.generated = OrderedDict()
         self.src_loc = src_loc
         self.origins = None
+        self.domains_propagated_up = {}
 
     def add_driver(self, signal, domain="comb"):
         assert isinstance(domain, str)
@@ -179,22 +180,37 @@ def _propagate_domains_up(self, hierarchy=("top",)):
                 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 subfrag, name, src_loc in self.subfragments:
+        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):
+    def _propagate_domains_down(self, hierarchy=("top",)):
         # For each domain defined in this fragment, ensure it also exists in all subfragments.
-        for subfrag, name, src_loc in self.subfragments:
+        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 in self.iter_domains():
                 if domain in subfrag.domains:
                     assert self.domains[domain] is subfrag.domains[domain]
                 else:
                     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()
+            subfrag._propagate_domains_down(hierarchy + (hier_name,))
 
     def _create_missing_domains(self, missing_domain, *, platform=None):
         from ._xfrm import DomainCollector

amaranth/hdl/_xfrm.py

@@ -1,3 +1,4 @@
+import warnings
 from abc import ABCMeta, abstractmethod
 from collections import OrderedDict
 from collections.abc import Iterable
@@ -539,11 +540,26 @@ def map_memory_ports(self, fragment, new_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 map_drivers(self, fragment, new_fragment):
@@ -569,6 +585,14 @@ 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)
 
 

docs/changes.rst

@@ -38,6 +38,7 @@ Apply the following changes to code written against Amaranth 0.4 to migrate it t
 * Replace imports of ``amaranth.asserts.{Assert, Assume, Cover}`` with imports from ``amaranth.hdl``
 * Remove any usage of ``name=`` with assertions, possibly replacing them with custom messages
 * Ensure all elaboratables are subclasses of :class:`Elaboratable`
+* Ensure clock domains aren't used outside the module that defines them, or its submodules; move clock domain definitions upwards in the hierarchy as necessary
 
 
 Implemented RFCs
@@ -53,6 +54,7 @@ Implemented RFCs
 .. _RFC 51: https://amaranth-lang.org/rfcs/0051-const-from-bits.html
 .. _RFC 53: https://amaranth-lang.org/rfcs/0053-ioport.html
 .. _RFC 55: https://amaranth-lang.org/rfcs/0055-lib-io.html
+.. _RFC 59: https://amaranth-lang.org/rfcs/0059-no-domain-upwards-propagation.html
 .. _RFC 62: https://amaranth-lang.org/rfcs/0062-memory-data.html
 
 * `RFC 17`_: Remove ``log2_int``
@@ -64,6 +66,7 @@ Implemented RFCs
 * `RFC 50`_: ``Print`` statement and string formatting
 * `RFC 51`_: Add ``ShapeCastable.from_bits`` and ``amaranth.lib.data.Const``
 * `RFC 53`_: Low-level I/O primitives
+* `RFC 59`_: Get rid of upwards propagation of clock domains
 
 
 Language changes
@@ -88,6 +91,7 @@ Language changes
 * Changed: :class:`Instance` IO ports now accept only IO values, not plain values. (`RFC 53`_)
 * Deprecated: :func:`amaranth.utils.log2_int`. (`RFC 17`_)
 * Deprecated: :class:`amaranth.hdl.Memory`. (`RFC 45`_)
+* Deprecated: upwards propagation of clock domains. (`RFC 59`_)
 * Removed: (deprecated in 0.4) :meth:`Const.normalize`. (`RFC 5`_)
 * Removed: (deprecated in 0.4) :class:`Repl`. (`RFC 10`_)
 * Removed: (deprecated in 0.4) :class:`ast.Sample`, :class:`ast.Past`, :class:`ast.Stable`, :class:`ast.Rose`, :class:`ast.Fell`.

tests/test_hdl_ir.py

@@ -668,6 +668,20 @@ 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):