Reset ports for `seq-mem` are ignored in `reset-insertion`

[calebmkim]

If I run:

fud e tests/correctness/seq-mem-dot-product.futil \
  -s verilog.data tests/correctness/seq-mem-dot-product.futil.data \
  --to dat --through verilog -v

I get 220, in the "out" value, as expected.

If I run (same thing but thru icarus-verilog not verilog):

fud e tests/correctness/seq-mem-dot-product.futil \
  -s verilog.data tests/correctness/seq-mem-dot-product.futil.data \
  --to dat --through icarus-verilog -v

I get 0, in the "out" value, which is not expected.

Does this reproduce for other people?

The iverilog version is 11.0, which is fine according to fud check. Another note is that I get a warning warning:

[fud] WARNING: Unknown option `verilog.top_module' for stage verilog 

when I run thru verilog.

[rachitnigam]

The warning is benign. Can you follow instruction for debugging compilation bugs to see if there is problem with any of the passes? If running with -p no-opt fixes the problem, then we know something is wrong in pass. Otherwise something is wrong in our Icarus test harness

[calebmkim]

-p no-opt doesn't fix the error.

One interesting thing that does fix the problem: in memories.sv, when we define seq_mem_d1, we have the following :

  // Write value to the memory
  always_ff @(posedge clk) begin
    if (!reset && write_en)
      mem[addr0] <= write_data;
  end 

If we remove the !reset, and just have if (write_en) then that fixes the problem. The reason why this is notable is because the !reset part was recently added in the TDST PR that was just merged.

So I think it's basically that the !reset condition is making it so that we're not writing to memory... but only for icarus-verilog, not for verilog.

[rachitnigam]

Oh weird, that just says that you shouldn't write anything to the memory while reset is asserted. Can you look at the waveform and see what's going on in there?

[calebmkim]

Sure; which wave viewer program should I use. Would just GTKWave (that's one of the ones listed in the documentation) be fine?

[rachitnigam]

If you use a Mac, you can use Scanscion too

[calebmkim]

Seems like we're just never triggering the .reset port of our seq-mems; the signal fir the port just says "x" for the entire time looking at the wave forms, and when I add a simple out.reset = reset to the Calyx file, it gives the correct/expected output.

This is probably something for the TDST pass? Is there a specific place in the TDST code where I should look?

[rachitnigam]

Could be that I forgot to add a @reset reset: 1 port to the memories in the futil file

[rachitnigam]

Huh, looks like I didn't. Can you look at the reset-insertion pass? It should add that assignment

[calebmkim]

The pass skips cells with the @external attribute, which includes the memories.

[rachitnigam]

Ugh, that might have to do with https://github.com/cucapra/calyx/issues/1034? I think it should be fine to not ignore the @external cells anymore? This would also explain the problem I was running into with #1338

[rachitnigam]

@calebmkim I might've missed this but did you open a PR to fix this?

[calebmkim]

I didn't because I read #1034 and I wasn't quite sure I fully understood the issue (maybe we can discuss more syncrhonously?).

However, if all that's required is to make a PR to re-enable reset insertion for external cells, then I can definitely do that.