Open ccelio opened 8 years ago
Do you know if any of the tools have problems handling [-1:0]? I thought they'd error out, but seems like they just trim the signal? Just noticed some of my old (functionally verified w/ Xilinx sim) Verilog had a bunch of those -1's...
I was only looking at outputs, not trying to run it through anything. But I think if [-1:0] is making it to the backend, there's something seriously wrong.
I agree :\
Actually I think this may just be a very simple bug. It looks like Reg(UInt) is pointing to:
def apply[T <: Data](outType: T) : T = Reg[T](Some(outType), None, None, None)
but I think the function should actually be:
def apply[T <: Data](outType: T) : T = Reg[T](Some(outType), Some(outType), None, None)
this is in the file Reg.scala, btw
when I make the change, the verilog output I get is:
module Hello(input clk,
output[7:0] io_z
);
wire[7:0] T1;
wire[4:0] T0;
reg [4:0] r;
`ifndef SYNTHESIS
// synthesis translate_off
integer initvar;
initial begin
#0.002;
r = {1{$random}};
end
// synthesis translate_on
`endif
assign io_z = T1;
assign T1 = {3'h0, T0};
assign T0 = r + 5'h1;
always @(posedge clk) begin
r <= 5'h1f;
end
endmodule
this solution is also the same as the one employed in RegNext apply methods.
Preferably though, Reg's apply would accept a parameter of type Class[Data] for the outType parameter. This would remove the ambiguity of the apply function calls.
OK, I think I have a good solution that won't break anything:
apply[T <: Data](outType: Class[Data], next: Option[T], init: Option[T], clock: Option[Clock])
apply[T <: Data](outType: Option[T], next: Option[T], init: Option[T], clock: Option[Clock])
as well as the other apply functions which use an Option[T]
instead of the more proper Class[Data]
Does everyone agree that this would be a good solution?
@JackDavidson Reg(UInt(31)) isn't supposed to assign 31 to anything; it's supposed to create a Reg of the same type as UInt(31) (i.e., UInt(width=5)). It is confusing but well defined.
@aswaterman alright, fair enough.
It looks like this is creating a lot of confusion though. Would it be possible to change the call Reg(UInt(##)) into Reg(UInt) and make Reg(UInt(##)) do what users are expecting?
The problem is other users expect UInt(31) to be a unsigned integer value of 31. Hence a width of 5.
TL,DR: There needs to be a width check on signals before generating the final code.
I've found the following circuit generates erroneous Verilog code (I didn't check C++).
It is purposefully BAD Chisel code, but the error should be caught earlier and not end up in Verilog!
Verilog, generates a broken
reg [-1:0] r
: