m-labs / migen

A Python toolbox for building complex digital hardware
https://m-labs.hk/migen
Other
1.2k stars 209 forks source link

Instance parameter value needlessly translated to narrowed width causes errors #252

Closed smosanu closed 2 years ago

smosanu commented 2 years ago

I am attempting to wrap a System Verilog module using the Instance(), and I pass two parameters (p_WA=WA, p_WB=WB) where WA=4 and WB=16 are python variables. This is translated into "efficient" Verilog.

# (
.WA(3'd4),
.WB(5'd16)
)

The problem is that I have a localparam in the module being wrapped, that is localparam WC = WA*WB

Strangely, Vivado simulation/synthesis evaluates the WA*WB to 0 instead of 64. If not for the needless "efficiency" in the number of bits of a parameter, it gets evaluated to 64. Is it possible to bypass the parameter formatting? I would like it to be:

# (
.WA(4),
.WB(16)
)

I see some options in other files such as C(0b101001, 6) or Instance.PreformattedParam("1") though I wasn't able to use them. My current solution is to not use a localparam, and pass WC as a param, but I would like to hide WC from the parameters. Thank you!

sbourdeauducq commented 2 years ago

I see some options in other files such as C(0b101001, 6) or Instance.PreformattedParam("1") though I wasn't able to use them.

Why not? p_WA= C(0b101001, 6) is supposed to work.

smosanu commented 2 years ago

C(0b00000000000000000000000000010000, 32) generates a constant (<migen.fhdl.structure.Constant object at 0x7f75147b4f28>), which I am not yet sure how to make use of, and passing it to p_WA results in an error raise TypeError("Width must be a strictly positive integer") because p_WA is an interface inout signal width.

But what is the reason for parameters' values to be converted to values defined as 5'd16 (i.e. why include the bit width)? A parameter in Verilog is not a hardware construct, is not a wire or a register, so what is the penalty, why not go nuts and use 64'd16 or gazillion'd16? or just skip the bit width and just use 16 alone? Every design choice has to be intentional and I don't get the intention behind this choice (or maybe I don't know of a specific Verilog particularity where this applies). Vivado messes up and takes 3'd4 x 5'd16 and evaluates that to zero (bad), but 4 x 16 is translated to 32'd4 x 32'd16 and evaluates to 32'64 (good).

It's not a big deal, maybe not worth spending time on this.

sbourdeauducq commented 2 years ago

passing it to p_WA results in an error raise TypeError("Width must be a strictly positive integer") because p_WA is an interface inout signal width.

What does that mean?

why not go nuts and use 64'd16 or gazillion'd16?

There will always be a point where that breaks. And if you use a huge number it may have negative consequences on downstream tools.

or just skip the bit width and just use 16 alone?

Verilog defaults to 32 if you omit the width.

smosanu commented 2 years ago

At https://github.com/m-labs/migen/blob/3ffd64c9b47619bd6689b44f29a8ed7c74365f14/migen/fhdl/verilog.py#L51 I replaced return str(node.nbits) + "'d" + str(node.value), False to str(node.value), False and so far so good. Thank you for all the help!

smosanu commented 2 years ago

After digging some more through the code, I finally figured out how to use Instance.PreformattedParam(WA), where WA is a python int, so generating a signal such as self.mysignal = Signal(WA) doesn't generate the error as before. Inside the Instance, for the parameters, use p_WA=Instance.PreformattedParam(WA) and all works. My mistake was to declare WA = Instance.PreformattedParam(4) and then passing WA to the Signal(). Migen has all I needed - no code changes required, just me being rusty as I get started with it. Sorry!