From 13316053e3311c6b1a8446ce0916b8ae538a8070 Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 19 Aug 2019 22:32:50 +0000 Subject: [PATCH] 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. --- nmigen/build/plat.py | 12 ++++----- nmigen/hdl/ir.py | 55 +++++++++++++++++--------------------- nmigen/hdl/xfrm.py | 21 +++++++++------ nmigen/test/test_hdl_ir.py | 5 ++-- nmigen/test/test_sim.py | 2 +- 5 files changed, 47 insertions(+), 48 deletions(-) diff --git a/nmigen/build/plat.py b/nmigen/build/plat.py index edb3049..af0edca 100644 --- 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 bd52226..1ad8f82 100644 --- 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 d143b41..8793c59 100644 --- 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 84bc2e2..8bdfd5d 100644 --- 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 d796e88..e7d759d 100644 --- 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() -- 2.30.2