amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.57k stars 174 forks source link

platform output bus bits in separate modules causes AssertionError #191

Closed nmigen-issue-migration closed 8 months ago

nmigen-issue-migration commented 5 years ago

Issue by dlharmon Friday Aug 30, 2019 at 23:16 GMT Originally opened as https://github.com/m-labs/nmigen/issues/190


I've worked around this by putting output buffer instances (OBUFTDS) for all 12 bits in the bus in a single module. It was failing when I used a module per output bit containing the buffer and serializer.

Gist of code to reproduce: https://gist.github.com/dlharmon/94f9b04b5d0bcf6dcd4a1bed1e194c0b

Traceback (most recent call last):
  File "AssertionError.py", line 71, in <module>
    SynthDigital1Platform().build(Synthesizer(), do_program=False)
  File "/home/dlharmon/python/nmigen/build/plat.py", line 66, in build
    plan = self.prepare(elaboratable, name, **kwargs)
  File "/home/dlharmon/python/nmigen/build/plat.py", line 128, in prepare
    fragment = fragment.prepare(ports=self.iter_ports(), missing_domain=lambda name: None)
  File "/home/dlharmon/python/nmigen/hdl/ir.py", line 548, in prepare
    fragment._propagate_ports(ports=(*ports, *new_ports), all_undef_as_ports=False)
  File "/home/dlharmon/python/nmigen/hdl/ir.py", line 469, in _propagate_ports
    self._prepare_use_def_graph(parent, level, uses, defs, ios, self)
  File "/home/dlharmon/python/nmigen/hdl/ir.py", line 443, in _prepare_use_def_graph
    subfrag._prepare_use_def_graph(parent, level, uses, defs, ios, top)
  File "/home/dlharmon/python/nmigen/hdl/ir.py", line 435, in _prepare_use_def_graph
    add_defs(value._lhs_signals())
  File "/home/dlharmon/python/nmigen/hdl/ir.py", line 405, in add_defs
    assert defs[sig] is self
AssertionError
nmigen-issue-migration commented 5 years ago

Comment by whitequark Friday Aug 30, 2019 at 23:21 GMT


Reduced testcase:

from nmigen import *
from nmigen.build import *
from nmigen.vendor.xilinx_7series import *

class se(Elaboratable):
    def __init__(self, p):
        self.p = p
    def elaborate(self, platform):
        m = Module()
        m.submodules.obuf = Instance("blah", o_O=self.p)
        return m

class Synthesizer(Elaboratable):
    def elaborate(self, platform):
        m = Module()
        d = platform.request("bus", 0)
        for i in range(len(d)):
            m.submodules["dacbit{}".format(i)] = se(d[i])
        return m

class SynthDigital1Platform(Xilinx7SeriesPlatform):
    device      = "xc7a15t"
    package     = "ftg256"
    speed       = "1"
    resources = [
        Resource("bus", 0, Pins("A8 B9 B10 B12 B15 C16 D14 E16 F15 G14 H16 J15", dir="o")),
    ]
    connectors = []

if __name__ == "__main__":
    SynthDigital1Platform().build(Synthesizer(), do_program=False)
nmigen-issue-migration commented 5 years ago

Comment by whitequark Friday Aug 30, 2019 at 23:22 GMT


By the way, why are you adding a "fake clock"? It's not necessary to have a default clock unless your code explicitly relies on it, you just need to make your own sync ClockDomain.

nmigen-issue-migration commented 5 years ago

Comment by dlharmon Friday Aug 30, 2019 at 23:26 GMT


The fake clock is now gone. I hadn't quite figured out how clock domains worked when I wrote that.

nmigen-issue-migration commented 5 years ago

Comment by whitequark Friday Aug 30, 2019 at 23:28 GMT


Ah, yeah. Documentation for that needs to be improved.

nmigen-issue-migration commented 5 years ago

Comment by whitequark Friday Aug 30, 2019 at 23:29 GMT


I'll fix this, but this might require a fairly significant redesign of use-def tracking (to make it per-bit rather than per-signal), so the fix is unlikely to be quick.

nmigen-issue-migration commented 5 years ago

Comment by dlharmon Friday Aug 30, 2019 at 23:34 GMT


I have implemented a fairly clean workaround. I mostly thought I'd report it for the sake of other users.

I started with nMigen a bit over a week ago and have converted two FPGA designs so far. It's a huge improvement over Verilog in many ways. Thanks for this great tool.

nmigen-issue-migration commented 5 years ago

Comment by whitequark Friday Aug 30, 2019 at 23:36 GMT


You're welcome! That sounds like many of my goals when making nMigen have been achieved.