chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
4.01k stars 600 forks source link

ReplSeqMem does not generate memory replacement for cells with specified ReadFirst/WriteFirst #1941

Open armleo opened 3 years ago

armleo commented 3 years ago

Hello,

Type of issue: bug report

Impact: API addition (no impact on existing code)

Other information When SyncReadMem is created with ReadFirst or WriteFirst specified SyncReadMem is not replaced by ReplSeqMem. It works fine if left default: Undefined

  1. SyncReadMem w/ ReadFirst or WriteFirst
  2. Compile with ReplSeqMem
  3. Expected behaviour memory replaced with macro and macro added to configuraiton file
  4. Current behaviour memory is not replaced and nothing is generated in configuration file

chisel3 latest stable (IDK if this needs to be reported to FIRRTL repo or here) If reproduction repo is required write a message in here and I will create it.

Thanks, Arman

sequencer commented 3 years ago

I just encounter this when demonstrating FIRRTL vlsi_mem_gen hours before!

As you can see from

https://github.com/chipsalliance/firrtl/blob/8bca41522cdc4b8ff69734cd159ce29f984d3290/src/main/scala/firrtl/passes/memlib/ToMemIR.scala#L18-L20

MemIR is not a part of FIRRTL IR, it is more like a WorkingIR specifically designed for memory macro extraction job. But currently memory macro extraction depends MemIR, its design only support “rw, w + r (read, write 1 cycle delay) and read-under-write "undefined." “

So I don’t know how to get this correct, seems we defined ReadFirst WriteFirst, but never use it in the real memory designs.

ekiwi commented 3 years ago

So I don’t know how to get this correct, seems we defined ReadFirst WriteFirst, but never use it in the real memory designs.

It is respected, when you generate a behavioral Verilog model with the default emitter and I believe the FPGA tools recognize it. For ASICs, I think everyone in Berkeley just assumes that the behavior will be undefined.

sequencer commented 3 years ago

Yes, I mean in ASICs seems FIRRTL cannot express the ReadFirst or WriteFirst behaviour since this leakage of MemIR.

armleo commented 2 years ago

Are there any updates regarding this issue?