I agree with the need for a new Chisel interface to create memories. However, why do we need an intrinsic for that instead of using the standard firrtl memories with their well-defined behavior in the backend?

I agree with the need for a new Chisel interface to create memories. However, why do we need an intrinsic for that instead of using the standard firrtl memories with their well-defined behavior in the backend?

Sorry for the late reply, I was in the hospital these days.

I just read the spec, I think this should be a good idea, but I think there are some issues in spec that we need concern:

• spec should take the additional port into consideration for BIST or DFT designs. CIRCT currently has a dirty way for it, see AddSeqMemPortAnnotation and AddSeqMemPortsFileAnnotation. Those are necessary for real world designs.
• The current firrtl memory model doesn't support memory init, which means it has a bad FPGA support.
• personally, I prefer treat memory as a instance, rather a memory type, since in the PnR, it's a hard marco, which should be treat as an instance.

I think this issue should follow #3162, in the future, we should add more metadata to firrtl spec, and switch to it. and then deprecate and remove the memory behavior model get rid of firrtl mem.

I thought about today, here is my solution to this:

• I still think we should use IntrinsicModule to instantiate a SRAM, rather than maintaining a Mem/SyncMem type in chisel, since unlike register, memory is always a hard macro and has corresponding IOs, it should be a Module/Instance, I think we should waive this 10y~ old design fault.
• For firrtl.mem, I think it should still be supported in firrtl spec and firtool: IMO, the correct behavior is: if not replacing macro, CIRCT should use the firrtl.mem to replace the intrinsic, and lower in the current way, I think this can solve your problem. In the repl-seq-mem flow(Or I prefer repl-mem) flow, this Module will be replace by the correct memory macros(even XPM for Xilinx FPGA).

The current firrtl memory model doesn't support memory init, which means it has a bad FPGA support.

We should totally add that.

• since unlike register, memory is always a hard macro and has corresponding IOs, it should be a Module/Instance, I think we should waive this 10y~ old design fault.

Most firrtl runs are for simulation and for that, there should be an accurate simulation model. This is why I think using a firrtl mem by default and replacing it with a hard macro for physical design makes the most sense. For FPGAs we can even make sure that the Verilog generated gets correctly inferred as BRAM thus making things work well by default.

• I still think we should use IntrinsicModule to instantiate a SRAM,

But what is the behavior of this? Ideally there should be one specification of how a Memory behaves and that the RTL designer has to know to make sure the high-level description is correct. For the mapping to different memory technologies you then just need to ensure that the physical macro is a valid refinement of the memory spec, i.e., it correctly implements all defined behaviors.

So to summarize: Yes, you could use an intrinsic for memory. However, I believe you then need to precisely define the semantics of this module and also add a behavioral default implementation for simulation. Which in the end means you just end up with something which is pretty much the same as the firrtl.mem today (plus some small additional features that we could also just add to the firrtl.mem).

Most firrtl runs are for simulation and for that, there should be an accurate simulation model. This is why I think using a firrtl mem by default and replacing it with a hard macro for physical design makes the most sense.

Like I said, I think a memory should be a instance rather a Mem/SyncMem type is better for PnR designs. I agree with using firrtl mem by default, which doesn't have conflict with the SRAMIntrinsic: during the lowering, if it is for PnR designs, it will goto repl-seq-mem, otherwise, a default behavior should use firrtl mem to replace this intmodule

But what is the behavior of this?

I just come across llvm/circt#5009, I think it should be directly map to this Op.

Yes, you could use an intrinsic for memory. However, I believe you then need to precisely define the semantics of this module and also add a behavioral default implementation for simulation. Which in the end means you just end up with something which is pretty much the same as the firrtl.mem today (plus some small additional features that we could also just add to the firrtl.mem).

Agree, the only reason I wanna use intmodule is because I wanna make Memory a instance rather than a chisel type. The backend implementation of this intmodule should be lowered to firrtl mem by default.