seq-mem outputs xs when not explicitly assigning to write_en

[nathanielnrn]

It seems like #1610 made a previously-harmless bug regarding default-assign (I can't find an associated issue) more dangerous.

Namely, before, if write_en was not assigned to, and therefore would (mistakenly?) default to x, we could still read successfully by setting content_en high. With the new seq_comb implementation:

  always_ff @(posedge clk) begin
    if (reset) begin
      read_out <= '0;
    end else if (content_en && !write_en) begin
      /* verilator lint_off WIDTH */
      read_out <= mem[addr0];
    end else if (content_en && write_en) begin
      // Explicitly clobber the read output when a write is performed
      read_out <= 'x;
    end else begin
      read_out <= read_out;
    end
  end

We need to explicitly assign write_en to be able to read anything at all, otherwise the x in write_en gets propagated to the signal read_data of our memories.

[andrewb1999]

Yeah I expected things like this to come up. There's not really a great solution to this other than figuring out the default assignment stuff. I have just been explicitly setting write_en to zero when calling content_en to do a read. Maybe we should just make write_en write_together(1) so it must be written to when content enable is set.

[rachitnigam]

@nathanielnrn could you provide a minimal example program where things break for you? We can then try to debug the default-assigns problem using it.

[nathanielnrn]

EDIT: A working reproducible example is listed in another comment below

---

So I thought something like this would reproduce the bug, where we drive write_en in the writer component but never assign it in main.

This program writes to a ref mem in writer and reads it in main.

import "primitives/core.futil";
import "primitives/binary_operators.futil";
import "primitives/memories/seq.futil";
component main () -> () {
    cells {
        real_mem = seq_mem_d1(32, 2, 1);
        my_writer = writer();
    }
    wires{
        group read1 {
            real_mem.content_en = 1'b1;
            real_mem.addr0 = 1'b0;
            read1[done] = real_mem.done;
        }
        group read2 {
            real_mem.content_en = 1'b1;
            real_mem.addr0 = 1'b1;
            read2[done] = real_mem.done;
        }
    }
    control{
        seq{
            invoke my_writer[ref_mem = real_mem]()();
            read1;
            read2;
        }
    }
}

component writer () -> () {
    cells {
        ref ref_mem = seq_mem_d1(32, 2, 1);
    }

    wires{
        group write1 {
            ref_mem.content_en = 1'b1;
            ref_mem.write_en  =1'b1;
            ref_mem.write_data = 32'b1;
            ref_mem.addr0 = 1'b0;
            write1[done] = ref_mem.done;
        }
        group write2 {
            ref_mem.content_en = 1'b1;
            ref_mem.write_en  = 1'b1;
            ref_mem.write_data = 32'd2;
            ref_mem.addr0 = 1'b1;
            write2[done] = ref_mem.done;
        }
    }

    control{
        seq{
            write1;
            write2;
        }
    }
}

However when looking at the vcds created with fud e ... --from calyx --to vcd --through icarus-verilog and with --through verilog we don't seem to get xs in the read_data output. So not sure where I'm going wrong.
So the above program behaves as I would expect it to when reading from the memory.

Will take me a couple of days to look at this further.

In case it's helpful, here is what I changed in the generator which I thought fixed the issue, but I may have been wrong about that.

cc @rachitnigam

[nathanielnrn]

Ok found a calyx program that reproduces these results. It should be noted that this only happens in icarus-verilog simulation, which I believe is because verilator treats x as zero-ish whereas icarus-verilog does not.

In particular doing fud e <...> --from calyx --to vcd --through icarus-verilog should produce output with xs in read_data.

The difference between this and the first attempt above is that the above reads in the main component, while this one reads in the reader component. So maybe the default-assignment pass correctly sets write_en to 0 if it is undeclared in the main component, but not in other components?

This program needs 2 groups because having a single group seems to create an unoptimizable circular combinational logic error in verilator, and hangs in icarus-verilog. See #1963.

import "primitives/core.futil";
import "primitives/binary_operators.futil";
import "primitives/memories/seq.futil";

component main () -> (){
    cells {
        my_reader = reader();
        my_writer = writer();
        real_mem = seq_mem_d1(32, 2, 1);
    }
    wires{}

    control{
        seq{
            invoke my_writer[ref_mem = real_mem]()();
            invoke my_reader[ref_mem=real_mem]()();
        }
    }
}



component reader () -> () {
    cells {
        ref ref_mem = seq_mem_d1(32, 2, 1);
        //my_writer = writer();
    }
    wires{
        group read1 {
            ref_mem.content_en = 1'b1;
            ref_mem.addr0 = 1'b0;
            read1[done] = ref_mem.done;
        }
        group read2 {
            ref_mem.content_en = 1'b1;
            ref_mem.addr0 = 1'b1;
            read2[done] = ref_mem.done;
        }
    }
    control{
        seq{
            read1;
            read2;
        }
    }
}

component writer () -> () {
    cells {
        ref ref_mem = seq_mem_d1(32, 2, 1);
    }

    wires{
        group write1 {
            ref_mem.content_en = 1'b1;
            ref_mem.write_en  =1'b1;
            ref_mem.write_data = 32'b1;
            ref_mem.addr0 = 1'b0;
            write1[done] = ref_mem.done;
        }
        group write2 {
            ref_mem.content_en = 1'b1;
            ref_mem.write_en  = 1'b1;
            ref_mem.write_data = 32'd2;
            ref_mem.addr0 = 1'b1;
            write2[done] = ref_mem.done;
        }
    }

    control{
        seq{
            write1;
            write2;
        }
    }
}

cc @rachitnigam