Invalid Reg width generation leads to bad bounds in generated Verilog

[ccelio]

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!

class Hello extends Module {        
  val io = new Bundle {             
    val z = UInt(OUTPUT, 8)         

    }                               

   val r = Reg(UInt(31)) // BAD style! width is unspecified, no initial value is set, and r  is never set again.         
   io.z := r + UInt(1)              
}                                   

Verilog, generates a broken reg [-1:0] r:

module Hello(input clk,
    output[7:0] io_z
);

  wire[7:0] T0;
  wire T1;
  wire T2;
  reg [-1:0] r; // <<<----- bad bounds!

`ifndef SYNTHESIS
  integer initvar;
  initial begin
    #0.002;
    r = {0{$random}};
  end
`endif

  assign io_z = T0;
  assign T0 = {7'h0, T1};
  assign T1 = T2 + 1'h1;
  assign T2 = {1'h0, r};

  always @(posedge clk) begin
    r <= r;
  end
endmodule

[shunshou]

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

[ccelio]

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.

[shunshou]

I agree :\

[JackDavidson]

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.

[JackDavidson]

OK, I think I have a good solution that won't break anything:

1. add an apply[T <: Data](outType: Class[Data], next: Option[T], init: Option[T], clock: Option[Clock])
2. point all the other apply functions to this new apply, using typeof(outType) to get the first parameter
3. depricate 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?

[aswaterman]

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

[JackDavidson]

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

[da-steve101]

The problem is other users expect UInt(31) to be a unsigned integer value of 31. Hence a width of 5.