Random memory initialization generates out of bounds address

[colinschmidt]

A read from a non-power of two memory can be initialized with an address that is out of the range of the memory. In vcs this results in an X output, potentially propagating throughout the rest of your design.

This showed up in one of the queues in the outer memory system of rocket-chip.

relevant firrtl looking like:

cmem ram : {client_xact_id : UInt<7>, addr_beat : UInt<2>, client_id : UInt<3>, is_builtin_type : UInt<1>, a_type : UInt<3>}[5]
...
infer mport T_326 = ram[T_274], clk

output verilog

assign ram_addr_beat_T_326_addr = T_274;
...
assign ram_addr_beat_T_326_data = ram_addr_beat[ram_addr_beat_T_326_addr];
...
initial begin
...
T_274 = {1{$random}};

[aswaterman]

I guess it could be emitted as something like

`ifdef SYNTHESIS
  assign rdata = mem[raddr];
`else
  assign rdata = raddr >= n ? {1{$random}} : mem[raddr];
`endif

[jackkoenig]

Andrew and I talked a bit about this, we think the right way to deal with it is to create a temporary node that is assigned the read data if the address is within range, garbage otherwise (through a validIf construct). Then all references to the read data are replaced with this node. eg:

    node readOutData = mem.reader.data

->

    node GEN_N = validif(lt(mem.reader.addr, UInt(MEM_SIZE)), mem.reader.data)
    node readOutData = GEN_N

[azidar]

If we do this strategy, then note that it is no longer legal to replace ValidIf(p, e) with e. I think this is ok, and the RemoveValidIf pass should replace ValidIf(p,e) with Mux(p,e,UInt(0)) (or something like that).

[aswaterman]

Hmm, Mux(p,e,0) will add a whole bunch of hardware in some cases. Let's not do that.

On Wednesday, May 4, 2016, Adam Izraelevitz notifications@github.com wrote:

If we do this strategy, then note that it is no longer legal to replace ValidIf(p, e) with e. I think this is ok, and the RemoveValidIf pass should replace ValidIf(p,e) with Mux(p,e,UInt(0)) (or something like that).

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/ucb-bar/firrtl/issues/155#issuecomment-216998700

[azidar]

Then perhaps we should not remove ValidIfs, and just emit them in Verilog like:

node GEN_N = validif(lt(mem.reader.addr, UInt(MEM_SIZE)), mem.reader.data)

becomes

`ifdef SYNTHESIS
  assign GEN_N = mem_reader_data;
`else
  assign GEN_N = mem.reader.addr < MEM_SIZE ? {1{$random}} : mem_reader_data;
`endif

[aswaterman]

I'm actually a little disinclined to change the ValidIf behavior in all cases... the status quo is working fine in practice, and doing this on all ValidIfs will result in a significant simulation performance hit. And Verilog simulation is already painfully slow.

Maybe we should just special-case this for Mems.

In either case, it's a little tricky to do this in such a way as to avoid introducing lint errors. Might have to do something like

`ifndef SYNTHESIS
  wire [31:0] GEN_N = {1{$random}};
  assign GEN_K = mem.reader.addr < MEM_SIZE ? GEN_N[15:0] : mem_reader_data;
`else
  assign GEN_K = mem_reader_data;
`endif

to avoid a width warning in the not-SYNTHESIS case and to avoid an unused wire warning in the SYNTHESIS case.

[jackkoenig]

I think that solution is fine, but shouldn't we do this for all validifs? What does validif even mean if we're not actually putting garbage data when the condition is false?

[azidar]

I think it should be a compiler option for the behavior of validif, because it is a tradeoff between performance and robustness that is within the legal spec. As such,

[aswaterman]

Or emit it with a separate `ifdef guard (e.g. RANDOMIZE_DISCONNECTED_NETS).

But the memory case should not be handled by the same ifdef, because in neither case should we introduce 'x into the simulation. That one should probably beifdef SYNTHESIS.

[colinschmidt]

There is a commit on the bringup-hwacha branch that fixes this

https://github.com/ucb-bar/firrtl/commit/fbc396e2cf42505c76188666153373471f98b00c