Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTLIL sync rules are Verilog-specific and should not be emitted #603

Closed
github-4o opened this issue Mar 31, 2021 · 2 comments · Fixed by #667
Closed

RTLIL sync rules are Verilog-specific and should not be emitted #603

github-4o opened this issue Mar 31, 2021 · 2 comments · Fixed by #667

Comments

@github-4o
Copy link

github-4o commented Mar 31, 2021

$ cat bug.py
from nmigen import *
from nmigen.cli import main


class bug(Elaboratable):
    def __init__(self):
        self.a = Signal()
        self.b = Signal()
        self.o = Signal()
        self.clk = Signal()
        self.rst = Signal()

    def ports(self):
        return [self.a, self.b, self.o, self.clk, self.rst]

    def elaborate(self, platform):
        m = Module()
        m.domains.sync = sync = ClockDomain('sync', async_reset = True)
        sync.clk = self.clk
        sync.rst = self.rst
        s = Signal()
        with m.If(s):
            with m.If(self.a):
                m.d.sync += s.eq(0)
            with m.Else():
                m.d.sync += s.eq(1)
        with m.Else():
            with m.If(self.b):
                m.d.sync += s.eq(1)
            with m.Else():
                m.d.sync += s.eq(0)
        m.d.comb += self.o.eq(s)
        return m

if __name__ == '__main__':
    uut = bug()
    main(uut, ports = uut.ports())
$ python3 bug.py generate -t v
Traceback (most recent call last):
  File "bug.py", line 37, in <module>
    main(uut, ports = uut.ports())
  File "/home/pi/repos/nmigen/nmigen/cli.py", line 78, in main
    main_runner(parser, parser.parse_args(), *args, **kwargs)
  File "/home/pi/repos/nmigen/nmigen/cli.py", line 62, in main_runner
    output = verilog.convert(fragment, name=name, ports=ports)
  File "/home/pi/repos/nmigen/nmigen/back/verilog.py", line 61, in convert
    return _convert_rtlil_text(rtlil_text, strip_internal_attrs=strip_internal_attrs)
  File "/home/pi/repos/nmigen/nmigen/back/verilog.py", line 51, in _convert_rtlil_text
    ignore_warnings=True)
  File "/home/pi/repos/nmigen/nmigen/_toolchain/yosys.py", line 186, in run
    return cls._process_result(popen.returncode, stdout, stderr, ignore_warnings, src_loc_at)
  File "/home/pi/repos/nmigen/nmigen/_toolchain/yosys.py", line 108, in _process_result
    raise YosysError(stderr.strip())
nmigen._toolchain.yosys.YosysError: ERROR: Multiple edge sensitive events found for this signal!

small changes to this code result in no yosys error:

  1. comb assignment of s, sync assignment of o
  2. simple reg m.d.sync += o.eq(i)
  3. passing False to async_reset
@whitequark whitequark changed the title yosys error on particular nmigen code RTLIL sync rules are Verilog-specific and should not be emitted Apr 10, 2021
@whitequark
Copy link
Member

Per discussion with @mwkmwkmwk, RTLIL sync rules are a Verilog construct that protrudes into RTLIL. The cause of the bug here is that proc_arst pass expects a certain construct specified in IEEE 1364.1, and we are not generating that exact construct.

Instead of using sync rules, we should emit $dff and $adff cells directly, and probably not even run any proc passes at all. This would be a much cleaner solution in every aspect, and not significantly hard to implement.

@ydnatag
Copy link

ydnatag commented Aug 19, 2021

I'm having a similar bug but Yosys doesn't rise an error, It just avoid the async reset. I workaround this for a while passing the signal through an auxiliary comb signal. But this workaround is not working for FSM state signal.

I have two questions:

  • Have you any idea how can i workaround this?
  • How difficult is to resolve this issue? Can you coach me to resolve it? How can I contribute?

Regards

Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes for now, pending changes at
a later date.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes for now, pending changes at
a later date.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes for now, pending changes at
a later date.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatoral processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
Lunaphied added a commit to Lunaphied/amaranth that referenced this issue Dec 18, 2021
We continue to emit combinatorial processes in RTLIL. This allows switch
statements in the HDL itself to be mapped to switch statements in RTLIL,
getting translated to Verilog case statements. This output has much
higher clarity than using `$mux` cells directly.

Fixes amaranth-lang#603.
whitequark pushed a commit that referenced this issue Jan 1, 2022
Avoiding emission of sync processes in RTLIL allows us to avoid a dependency on
matching the behavior expected by Yosys, which generally expects sync processes
in RTLIL to match those emitted by the output from its own Verilog parser.
This also simplifies the logic used in emitting RTLIL overall.

Combinatorial processes are still emitted however. Without these the RTLIL does
not have a high-level understanding of Switch statements, which significantly
diminishes the quality of emitted Verilog, as these are converted to `$mux`
cells in Yosys, which become `?` constructs when converted back to Verilog.

Fixes #603.
Fixes #672.
@whitequark whitequark added this to the 0.4 milestone Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants