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

Jump to bottom

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
Labels
backend:rtlil bug
Milestone
0.4

Comments

@github-4o
Copy link

github-4o commented Mar 31, 2021 •
edited by whitequark 

$ 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
@Lunaphied
back.rtlil: avoid sync process emission in rtlil.
557ecc7 
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
@Lunaphied
back.rtlil: avoid sync process emission in rtlil.
c6d8ed4 
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
@Lunaphied
back.rtlil: avoid sync process emission in rtlil.
9757fc3 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
3ed767c 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
8a8f0ec 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
bd1fbfc 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
9e8a8b5 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
0040d6f 
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
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
abc2d06 
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.
@Lunaphied Lunaphied mentioned this issue Dec 18, 2021
Merged
whitequark pushed a commit that referenced this issue Jan 1, 2022
@Lunaphied
back.rtlil: avoid sync process emission in RTLIL.
5a4d45b 
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
@povik povik mentioned this issue Feb 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:rtlil bug
Milestone
0.4
Development

Successfully merging a pull request may close this issue.

back.rtlil: avoid sync process emission in RTLIL. Lunaphied/amaranth
3 participants
@whitequark @ydnatag @github-4o