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

[WRansohoff]

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]

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]

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]

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]

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]

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

[whitequark]

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

[WRansohoff]

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]

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]

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]

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]

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]

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]

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]

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]

Did you link a wrong commit?

[WRansohoff]

yes, sorry.

WRansohoff@1d8ea9e

[whitequark]

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

[whitequark]

@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]

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).