build.plat, hdl.ir: coordinate missing domain creation.

Platform.prepare() was completely broken after addition of local
clock domains, and only really worked before by a series of
accidents because there was a circular dependency between creation
of missing domains, fragment preparation, and insertion of pin
subfragments.

This commit untangles the dependency by adding a separate public
method Fragment.create_missing_domains(), used in build.plat.

It also makes DomainCollector consider both used and defined domains,
such that it will work on fragments before domain propagation, since
create_missing_domains() can be called by user code before prepare().

The fragment driving missing clock domain is not flattened anymore,
because flattening does not work well combined with local domains.

diff --git a/nmigen/build/plat.py b/nmigen/build/plat.py
index edb3049795bc1cd137093e0b7a1efe2b3271adbc..af0edca19f04a1b17e73e39f1ecef656555672fe 100644 (file)
--- a/nmigen/build/plat.py
+++ b/nmigen/build/plat.py
@@ -58,11 +58,11 @@ class Platform(ResourceManager, metaclass=ABCMeta):
             raise TypeError("File contents must be str, bytes, or a file-like object")
         self.extra_files[filename] = content
 
-    def build(self, fragment, name="top",
+    def build(self, elaboratable, name="top",
               build_dir="build", do_build=True,
               program_opts=None, do_program=False,
               **kwargs):
-        plan = self.prepare(fragment, name, **kwargs)
+        plan = self.prepare(elaboratable, name, **kwargs)
         if not do_build:
             return plan
 
@@ -90,13 +90,12 @@ class Platform(ResourceManager, metaclass=ABCMeta):
                 m.d.comb += ResetSignal("sync").eq(rst_i)
             return m
 
-    def prepare(self, fragment, name="top", **kwargs):
+    def prepare(self, elaboratable, name="top", **kwargs):
         assert not self._prepared
         self._prepared = True
 
-        fragment = Fragment.get(fragment, self)
-        fragment = fragment.prepare(ports=list(self.iter_ports()),
-                                    missing_domain=self.create_missing_domain)
+        fragment = Fragment.get(elaboratable, self)
+        fragment.create_missing_domains(self.create_missing_domain)
 
         def add_pin_fragment(pin, pin_fragment):
             pin_fragment = Fragment.get(pin_fragment, self)
@@ -125,6 +124,7 @@ class Platform(ResourceManager, metaclass=ABCMeta):
                 add_pin_fragment(pin,
                     self.get_diff_input_output(pin, p_port, n_port, attrs, invert))
 
+        fragment = fragment.prepare(ports=self.iter_ports(), missing_domain=lambda name: None)
         return self.toolchain_prepare(fragment, name, **kwargs)
 
     @abstractmethod

diff --git a/nmigen/hdl/ir.py b/nmigen/hdl/ir.py
index bd52226cf8001a90ef1956d7d6aff01f210dd658..1ad8f82958b049bcab1c22c62527eec8681e30d6 100644 (file)
--- a/nmigen/hdl/ir.py
+++ b/nmigen/hdl/ir.py
@@ -355,46 +355,41 @@ class Fragment:
 
             subfrag._propagate_domains_down()
 
-    def _create_missing_domains(self, missing_domain):
+    def create_missing_domains(self, missing_domain):
         from .xfrm import DomainCollector
 
+        collector = DomainCollector()
+        collector(self)
+
         new_domains = []
-        for domain_name in DomainCollector()(self):
+        for domain_name in collector.used_domains - collector.defined_domains:
             if domain_name is None:
                 continue
-            if domain_name not in self.domains:
-                value = missing_domain(domain_name)
-                if value is None:
-                    raise DomainError("Domain '{}' is used but not defined".format(domain_name))
-                if type(value) is ClockDomain:
-                    self.add_domains(value)
-                    # And expose ports on the newly added clock domain, since it is added directly
-                    # and there was no chance to add any logic driving it.
-                    new_domains.append(value)
-                else:
-                    new_fragment = Fragment.get(value, platform=None)
-                    if domain_name not in new_fragment.domains:
-                        defined = new_fragment.domains.keys()
-                        raise DomainError(
-                            "Fragment returned by missing domain callback does not define "
-                            "requested domain '{}' (defines {})."
-                            .format(domain_name, ", ".join("'{}'".format(n) for n in defined)))
-                    new_fragment.flatten = True
-                    self.add_subfragment(new_fragment)
-                    self.add_domains(new_fragment.domains.values())
+            value = missing_domain(domain_name)
+            if value is None:
+                raise DomainError("Domain '{}' is used but not defined".format(domain_name))
+            if type(value) is ClockDomain:
+                self.add_domains(value)
+                # And expose ports on the newly added clock domain, since it is added directly
+                # and there was no chance to add any logic driving it.
+                new_domains.append(value)
+            else:
+                new_fragment = Fragment.get(value, platform=None)
+                if domain_name not in new_fragment.domains:
+                    defined = new_fragment.domains.keys()
+                    raise DomainError(
+                        "Fragment returned by missing domain callback does not define "
+                        "requested domain '{}' (defines {})."
+                        .format(domain_name, ", ".join("'{}'".format(n) for n in defined)))
+                self.add_subfragment(new_fragment, "cd_{}".format(domain_name))
         return new_domains
 
     def _propagate_domains(self, missing_domain):
+        new_domains = self.create_missing_domains(missing_domain)
         self._propagate_domains_up()
-        new_domains = self._create_missing_domains(missing_domain)
         self._propagate_domains_down()
         return new_domains
 
-    def _lower_domain_signals(self):
-        from .xfrm import DomainLowerer
-
-        return DomainLowerer()(self)
-
     def _prepare_use_def_graph(self, parent, level, uses, defs, ios, top):
         def add_uses(*sigs, self=self):
             for sig in flatten(sigs):
@@ -536,12 +531,12 @@ class Fragment:
                 self.add_ports(sig, dir="i")
 
     def prepare(self, ports=None, missing_domain=lambda name: ClockDomain(name)):
-        from .xfrm import SampleLowerer
+        from .xfrm import SampleLowerer, DomainLowerer
 
         fragment = SampleLowerer()(self)
         new_domains = fragment._propagate_domains(missing_domain)
         fragment._resolve_hierarchy_conflicts()
-        fragment = fragment._lower_domain_signals()
+        fragment = DomainLowerer()(fragment)
         if ports is None:
             fragment._propagate_ports(ports=(), all_undef_as_ports=True)
         else:

diff --git a/nmigen/hdl/xfrm.py b/nmigen/hdl/xfrm.py
index d143b41cc1ad0eff136b8e4e5ab1ee8488fb40d1..8793c5996d33ab544da82f2242366f1e0c21fd92 100644 (file)
--- a/nmigen/hdl/xfrm.py
+++ b/nmigen/hdl/xfrm.py
@@ -336,12 +336,16 @@ class TransformedElaboratable(Elaboratable):
 
 class DomainCollector(ValueVisitor, StatementVisitor):
     def __init__(self):
-        self.domains = set()
+        self.used_domains = set()
+        self.defined_domains = set()
         self._local_domains = set()
 
-    def _add_domain(self, domain_name):
-        if domain_name not in self._local_domains:
-            self.domains.add(domain_name)
+    def _add_used_domain(self, domain_name):
+        if domain_name is None:
+            return
+        if domain_name in self._local_domains:
+            return
+        self.used_domains.add(domain_name)
 
     def on_ignore(self, value):
         pass
@@ -352,10 +356,10 @@ class DomainCollector(ValueVisitor, StatementVisitor):
     on_Signal = on_ignore
 
     def on_ClockSignal(self, value):
-        self._add_domain(value.domain)
+        self._add_used_domain(value.domain)
 
     def on_ResetSignal(self, value):
-        self._add_domain(value.domain)
+        self._add_used_domain(value.domain)
 
     on_Record = on_ignore
 
@@ -416,10 +420,12 @@ class DomainCollector(ValueVisitor, StatementVisitor):
         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.on_statements(fragment.statements)
         for domain_name in fragment.drivers:
-            self._add_domain(domain_name)
+            self._add_used_domain(domain_name)
         for subfragment, name in fragment.subfragments:
             self.on_fragment(subfragment)
 
@@ -427,7 +433,6 @@ class DomainCollector(ValueVisitor, StatementVisitor):
 
     def __call__(self, fragment):
         self.on_fragment(fragment)
-        return self.domains
 
 
 class DomainRenamer(FragmentTransformer, ValueTransformer, StatementTransformer):

diff --git a/nmigen/test/test_hdl_ir.py b/nmigen/test/test_hdl_ir.py
index 84bc2e2c458f147cb8ae4ab1fa1d621b947567b7..8bdfd5da35cd7959661aa6ddc5e9ce2593c642d4 100644 (file)
--- a/nmigen/test/test_hdl_ir.py
+++ b/nmigen/test/test_hdl_ir.py
@@ -428,9 +428,8 @@ class FragmentDomainsTestCase(FHDLTestCase):
         self.assertEqual(f1.domains["sync"], f2.domains["sync"])
         self.assertEqual(new_domains, [])
         self.assertEqual(f1.subfragments, [
-            (f2, None)
+            (f2, "cd_sync")
         ])
-        self.assertTrue(f2.flatten)
 
     def test_propagate_create_missing_fragment_many_domains(self):
         s1 = Signal()
@@ -448,7 +447,7 @@ class FragmentDomainsTestCase(FHDLTestCase):
         self.assertEqual(f1.domains["sync"], f2.domains["sync"])
         self.assertEqual(new_domains, [])
         self.assertEqual(f1.subfragments, [
-            (f2, None)
+            (f2, "cd_sync")
         ])
 
     def test_propagate_create_missing_fragment_wrong(self):

diff --git a/nmigen/test/test_sim.py b/nmigen/test/test_sim.py
index d796e8817909279b720e1f3a9adc6813ac65bb4b..e7d759dbaccc5b1d002873754787e5c94140fd18 100644 (file)
--- a/nmigen/test/test_sim.py
+++ b/nmigen/test/test_sim.py
@@ -250,7 +250,7 @@ class SimulatorUnitTestCase(FHDLTestCase):
 class SimulatorIntegrationTestCase(FHDLTestCase):
     @contextmanager
     def assertSimulation(self, module, deadline=None):
-        with Simulator(module.elaborate(platform=None)) as sim:
+        with Simulator(module) as sim:
             yield sim
             if deadline is None:
                 sim.run()