Skip to content

nmigen generates invalid RTLIL with negative shifts #302

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.

Sign up for GitHub

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
Closed
nmigen-issue-migration opened this issue Jan 11, 2020 · 3 comments
Closed

nmigen generates invalid RTLIL with negative shifts #302

nmigen-issue-migration opened this issue Jan 11, 2020 · 3 comments
Labels
backend:rtlil bug
Milestone
0.2

Comments

@nmigen-issue-migration
Copy link

Issue by ZirconiumX
Saturday Jan 11, 2020 at 17:13 GMT
Originally opened as m-labs/nmigen#302

from nmigen import *
from nmigen.back import verilog

class Test(Elaboratable):
    def __init__(self):
        self.i = Signal(8)
        self.o = Signal(8)

    def elaborate(self, platform):
        m = Module()

        m.d.comb += self.o.eq(self.i << -3)

        return m

print(verilog.convert(Test()))

produces

Traceback (most recent call last):
  File "test.py", line 16, in <module>
    print(verilog.convert(Test()))
  File "/usr/local/lib/python3.7/dist-packages/nmigen-0.2.dev13+g476ce15-py3.7.egg/nmigen/back/verilog.py", line 77, in convert
    return _convert_rtlil_text(rtlil_text, strip_internal_attrs=strip_internal_attrs)
  File "/usr/local/lib/python3.7/dist-packages/nmigen-0.2.dev13+g476ce15-py3.7.egg/nmigen/back/verilog.py", line 65, in _convert_rtlil_text
    raise YosysError(error.strip())
nmigen.back.verilog.YosysError: ERROR: Found error in internal cell \top.$3 ($sshl) at kernel/rtlil.cc:790:
  attribute \src "test.py:12"
  cell $sshl $3
    parameter \Y_WIDTH 4'1011
    parameter \B_WIDTH 2'11
    parameter \B_SIGNED 1'1
    parameter \A_WIDTH 4'1000
    parameter \A_SIGNED 1'0
    connect \Y $2
    connect \B 3'101
    connect \A \i
  end
@nmigen-issue-migration nmigen-issue-migration added this to the 0.2 milestone Jan 27, 2020
@whitequark
Copy link
Member

The underlying cause is that RTLIL never supported signed shifts, but erroneously did not reject them in the validator. After YosysHQ/yosys#1551 got merged, our generated RTLIL became invalid.

There are two possible fixes here:

  • Transparently translate signed shifts into a mux fed by a left shift and a right shift cell;
  • Reject shifts by signed values.

Any opinions?

@Ravenslofty
Copy link
Contributor

Honestly, I'd say that shifts by negative numbers definitely point to a bug. Really, you could get the equivalent of option 1 with a rotate operation, which I think would be clearer in semantics.

@whitequark
Copy link
Member

Everyone thinks we should ban them, so let's just do that.

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.2
Development

No branches or pull requests
3 participants
@whitequark @Ravenslofty @nmigen-issue-migration