chipsalliance / chisel

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

Memory should be treated as public #4485

Open sequencer opened 1 week ago

sequencer commented 1 week ago

Type of issue: Bug Report

Please provide the steps to reproduce the problem: I assign 2 readwrite ports(aka Dual Port in ASIC) in the chisel(as well as the metadata), I tied off the writeEnable of port 0, CIRCT is too smart to find it and infer the memory to a wrong shape: 1 read 1 readwrite.

What is the current behavior? Chisel is doing right to generate the behavior memory; CIRCT is doing right to constant propagation and infer the memory;

But we get the wrong type of memory, because the semantic of SRAM is private and the interface can be optimized out.

What is the expected behavior?

shape and interface of Memory should never change.

We should:

poemonsense commented 1 week ago

I think semantics for memories are difficult to be correctly inferred. I suggest rethinking the optimization boundaries or Chisel descriptions for memories. Probably Chisel should not (or not be able to) perform too many optimizations on memories.

What's the expected optimization output (how many write ports) for the following memory? I don't think Chisel/CIRCT is theoretically able to infer the number of ports correctly.

val sram = SyncReadMem(...)

when (cond) { sram.write(...) }

// Another write in another when with a NOT cond
when (!cond) { sram.write(...) }

// What about a more complicated example
// io.cond2 and io.cond1 is assigned outside the module scope with cond and !cond
val sram2 = SyncReadMem(...)
when (io.cond1) { sram2.write(...) }
when (io.cond2) { sram2.write(...) }