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

nMigen should avoid emitting very large wires that cause issues in Yosys #341

Closed
WRansohoff opened this issue Mar 28, 2020 · 19 comments
Closed

Comments

@WRansohoff
Copy link
Contributor

The following code seems to cause a YoSys parser error when it is built:

from nmigen import *
from nmigen_boards.icestick import *

class Parsicide( Elaboratable ):
  def __init__( self ):
    self.a = Signal( 32, reset = 0 )
    self.b = Signal( 32, reset = 0 )
    self.y = Signal( 32, reset = 0 )
  def elaborate( self, platform ):
    m = Module()
    m.d.comb += self.y.eq( self.a << self.b )
    return m

if __name__ == "__main__":
  ICEStickPlatform().build( Parsicide(), do_program = False )

This is the error I get:

Traceback (most recent call last):
  File "yosys_err.py", line 15, in <module>
    ICEStickPlatform().build( Parsicide(), do_program = False )
  File "[...]/nmigen/build/plat.py", line 78, in build
    plan = self.prepare(elaboratable, name, **kwargs)
  File "[...]/nmigen/build/plat.py", line 151, in prepare
    return self.toolchain_prepare(fragment, name, **kwargs)
  File "[...]/nmigen/build/plat.py", line 398, in toolchain_prepare
    render(content_tpl, origin=content_tpl))
  File "[...]/nmigen/build/plat.py", line 392, in render
    "autogenerated": autogenerated,
  File "[...]/jinja2/environment.py", line 1090, in render
    self.environment.handle_exception()
  File "[...]/jinja2/environment.py", line 832, in handle_exception
    reraise(*rewrite_traceback_stack(source=source))
  File "[...]/jinja2/_compat.py", line 28, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 2, in top-level template code
  File "[...]/nmigen/build/plat.py", line 294, in emit_debug_verilog
    strip_internal_attrs=False, write_verilog_opts=opts)
  File "[...]/nmigen/back/verilog.py", line 67, in _convert_rtlil_text
    raise YosysError(error.strip())
nmigen.back.verilog.YosysError: ERROR: Parser error in line 29: invalid slice

Replacing the left shift with a right shift results in the module building without problems (besides the 'no clocks' warnings). I think I'm using a pretty recent version of YoSys: 0.9+1706 (git sha1 ed4fa19b)

@WRansohoff
Copy link
Contributor Author

Here's the generated RTLIL, but unfortunately I don't know how to parse it any better than YoSys does:

# Convert nMigen's RTLIL to readable Verilog.
read_ilang <<rtlil
attribute \generator "nMigen"
attribute \top 1
attribute \nmigen.hierarchy "top"
module \top
  attribute \src "yosys_err.py:8"
  wire width 32 \y
  attribute \src "yosys_err.py:11"
  wire width 4294967327 $1
  attribute \src "yosys_err.py:6"
  wire width 32 \a
  attribute \src "yosys_err.py:7"
  wire width 32 \b
  attribute \src "yosys_err.py:11"
  wire width 4294967327 $2
  attribute \src "yosys_err.py:11"
  cell $sshl $3
    parameter \A_SIGNED 1'0 
    parameter \A_WIDTH 6'100000
    parameter \B_SIGNED 1'0 
    parameter \B_WIDTH 6'100000
    parameter \Y_WIDTH 33'100000000000000000000000000011111
    connect \A \a
    connect \B \b
    connect \Y $2
  end 
  connect $1 $2
  process $group_0
    assign \y 32'00000000000000000000000000000000
    assign \y $1 [31:0]
    sync init
  end 
  connect \a 32'00000000000000000000000000000000
  connect \b 32'00000000000000000000000000000000
end

@whitequark whitequark changed the title Left-shift operation causes YoSys parser error Left-shift operation causes Yosys parser error Mar 28, 2020
@whitequark
Copy link
Member

The cause of this error is some truly enormous wires here:

  attribute \src "x.py:12"
  wire width 4294967327 $1
  attribute \src "x.py:12"
  wire width 4294967327 $2

This happens because the result of a left shift is sized appropriately to hold the result no matter what the shift amount is, i.e. it has the size of len(lhs) + max(rhs) bits. In this case, since the rhs is 32-bit, that is a lot.

I'm inclined to say this isn't a bug with nMigen, since that's the intended semantics, even if unexpected. It tickles a bug in handling very large slices in Yosys, which is unfortunate, but not necessarily trivial to fix. What I think we should do here is to make the RTLIL backend error out instead of creating very large wires, say anything over 1 million bit. (Or perhaps warn rather than error out.)

@WRansohoff
Copy link
Contributor Author

Ah, that makes sense - thanks. The same issue occurs with this left-shift code, though:

m.d.comb += self.y.eq( self.a << ( self.b & 0x1F ) )

Shouldn't max(rhs) = 32 in that case? Or should a different syntax be used to shorten the b Signal?

@whitequark
Copy link
Member

Shouldn't max(rhs) = 32 in that case?

nMigen never uses constants to determine result sizes. This would lead to very surprising results in certain corner cases, such as if you had a piece of generic code that sometimes takes a constant and sometimes a variable, or just different constants.

Or should a different syntax be used to shorten the b Signal?

Try b[:5].

@WRansohoff
Copy link
Contributor Author

Oh, interesting. b[:5] seems to work, thanks!

@whitequark whitequark changed the title Left-shift operation causes Yosys parser error nMigen should avoid emitting very large wires that cause issues in Yosys Mar 28, 2020
@whitequark whitequark reopened this Mar 28, 2020
@whitequark
Copy link
Member

Let's keep this open, since it is a usability bug.

@WRansohoff
Copy link
Contributor Author

WRansohoff commented Mar 28, 2020

Then would something like this work for the RTLIL backend warning? Maybe if it had its own Warning class like the DriverConflict and UnusedElaboratable ones? (And with fewer typos)

WRansohoff@cf45394

@whitequark
Copy link
Member

No, not really. I think it should be a hard error until it's fixed upstream in Yosys (btw, could you report it there please?) and it should probably be in def wire(...):.

@Ravenslofty
Copy link
Contributor

I personally would add a note for shifts in this instance that looks like "if this was unintentional, consider using slice notation to shorten the width of the right hand side expression".

@whitequark
Copy link
Member

I agree that we could also do that for shifts where the result ends up larger than the LHS. The question is, how do we suppress that warning? What would be the syntax for that?

@Ravenslofty
Copy link
Contributor

Does nMigen have a way to zero/sign-extend a Value to a given width that's more elegant than Cat(Repl(0, target_width - len(foo)), foo)/Cat(Repl(foo[-1], target_width - len(foo)), foo) (in rough pseudocode)? If the lint triggers because the result is wider than the LHS, then the options to suppress it are to shrink the RHS or lengthen the LHS. Personally I would argue that the explicitness of those options is not a bad thing.

@WRansohoff
Copy link
Contributor Author

It looks like you can suppress individual types of warnings when the code is simulated or built like this:

with warnings.catch_warnings():
  warnings.filterwarnings( "ignore", category = DriverConflict )
  [ code to run without warnings... ]

There's also python -W ignore::nmigen.hdl.ir.DriverConflict file.py, but I haven't been able to get that to work; I get "Invalid -W option ignored: invalid module name: 'nmigen.hdl.ir'", and setting $PYTHONPATH to include the directory where nmigen is installed only seems to cause more confusing errors...

On the bright side, it should be easier to raise an Exception than find a clean way to handle warnings.

@whitequark
Copy link
Member

Does nMigen have a way to zero/sign-extend a Value to a given width

To a given specific width? You could do something like + C(0, width) but it's hacky. In general this operation is very rarely (if ever) useful.

then the options to suppress it are to shrink the RHS or lengthen the LHS

Indeed. I think that silencing the warning if the RHS is indexed, something like a << b[:1], is pretty a reasonable solution, but I'm happy to hear arguments against it.

It looks like you can suppress individual types of warnings when the code is simulated or built like this:

Yes, but that's far too verbose for a relatively high false positive rate warning like this. Such warnings should be possible to suppress with natural-looking constructs like the one I suggested above.

@WRansohoff
Copy link
Contributor Author

Okay, I reported this to Yosys here - hopefully I didn't misunderstand the nature of the issue:

YosysHQ/yosys#1838

How about this, for raising an error with oversized wires?

WRansohoff@2d1e12d

Unfortunately, that doesn't seem to indicate where the error occurs in the 'pre-generation' code or the RTLIL that would otherwise be generated, so it might not be too helpful if someone sees it after making a large change.

@whitequark
Copy link
Member

Did you link a wrong commit?

@WRansohoff
Copy link
Contributor Author

yes, sorry.

WRansohoff@1d8ea9e

@whitequark
Copy link
Member

Please leave it to me, this is one of those changes that are faster to implement than to properly discuss and review.

@whitequark whitequark added this to the 0.3 milestone Apr 13, 2020
@whitequark
Copy link
Member

@WRansohoff Thank you for the report and the discussion. As you can see in the commit, I had to look up the limits we are bound with in the Verilog standard and the Yosys source code. Although I could have guided you through that during review, it would be equivalent to writing the code myself, just with more steps.

@WRansohoff
Copy link
Contributor Author

Thank you for the fix! And I understand, I probably won't know enough to make meaningful contributions to HDL tools for a long time (if ever).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants