Module with multiple memories have no readmem statements

[carlosedp]

Checklist

• Did you specify the current behavior?
• Did you specify the expected behavior?
• Did you provide a code example showing the problem?
• Did you describe your environment?
• Did you specify relevant external information?

What is the current behavior?

I noticed that having multiple memory objects, the readmem statements are not generated for neither memory.

Just commenting out the mem2 and loadStorePort2 objects, mem gets it's readmemh.

Here is a sample code I'm using:

package utils

import chisel3._
import chisel3.util.log2Ceil
import chisel3.util.experimental.loadMemoryFromFileInline
import chisel3.experimental.{annotate, ChiselAnnotation}
import firrtl.annotations.MemorySynthInit

class MultiMem extends Module {
  val io = IO(new Bundle() {
    val loadStorePort = Flipped(new MemoryPort(32, 32, true))
    val loadStorePort2 = Flipped(new MemoryPort(32, 32, true))
  })
  annotate(new ChiselAnnotation {
    override def toFirrtl =
      MemorySynthInit
  })
  val mem1 =
    Module(
      new DualPortRAM(
        sizeBytes = 32 * 1024,
        bitWidth = 32,
        memoryFile = "sample.hex"
      )
    )
  mem1.io.loadStorePort <> io.loadStorePort

  val mem2 =
    Module(
      new DualPortRAM(
        sizeBytes = 32 * 1024,
        bitWidth = 32,
        memoryFile = "sample.hex"
      )
    )
  mem2.io.loadStorePort <> io.loadStorePort2
}

class MemoryPort(val bitWidth: Int, val words: Int, val rw: Boolean)
    extends Bundle {
  val addr = Output(UInt(log2Ceil(words).W))
  val readData = Input(UInt(bitWidth.W))

  val writeEnable = if (rw) Some(Output(Bool())) else None
  val writeData = if (rw) Some(Output(UInt(bitWidth.W))) else None
}

class DualPortRAM(
    sizeBytes: Int = 1,
    bitWidth: Int = 32,
    memoryFile: String = ""
) extends Module {
  val words = sizeBytes / bitWidth
  val io = IO(new Bundle() {
    val loadStorePort = Flipped(new MemoryPort(bitWidth, words, true))
  })
  
  val mem = SyncReadMem(words, UInt(bitWidth.W))

  if (memoryFile.trim().nonEmpty) {
    println(s"  Load memory file: " + memoryFile)
    loadMemoryFromFileInline(mem, memoryFile)
  }

  io.loadStorePort.readData := mem.read(io.loadStorePort.addr)

  when(io.loadStorePort.writeEnable.get) {
    mem.write(io.loadStorePort.addr, io.loadStorePort.writeData.get)
  }
}

object MultiMem extends App {
  (new chisel3.stage.ChiselStage).emitVerilog(
    new MultiMem(),
    Array("-X", "verilog") ++ args
  )
}

Am I doing something wrong in my code or really is there is a problem in generation?
Is there a real case use for this?

What is the expected behavior?

Both memories have their readmem statements.

Steps to Reproduce

Code below

Your environment

Chisel and Firrtl SNAPSHOT.

External Information

[Quarky93]

I think I have the same issue, try using --no-dedup option.

[carlosedp]

I'll test this out but believe the readmem should be generated for all memories since they could contain different content and initialization right?

[Quarky93]

Yea definitely, I believe it's a more general problem where any annotation is getting lost after duplicate modules are merged/deduped.
chipsalliance/chisel#1846

[carlosedp]

Confirming that with --no-dedup the readmem statements are generated correctly!

[Quarky93]

yea, tho it's still a big problem for me at least, since I have several thousand replicated modules, so using no-dedup is not really an option for me. I'd be happy to try to fix this, but i have no idea where to start... @jackkoenig @seldridge ?

[carlosedp]

@ekiwi any pointers on how to tackle this? Using no-dedup is the way?

[ekiwi]

@ekiwi any pointers on how to tackle this? Using no-dedup is the way?

This seems to be a bug, but I do not understand enough about how annotations are supposed to interact with Dedup to know how to address it. @jackkoenig and @seldridge should know more about this part of firrtl.

[jackkoenig]

I poked around this a bit and I think solving the fundamental issue is a bit tricky but doable. It mainly just requires a bit of cleverness (and time). Perhaps a more patchy solution to unblock people is to do a manual deduplication of these emitter annotations in the emitter. It doesn't solve the issue for everything but at least would unblock people here.

[carlosedp]

Nice! great it's on the milestone too :)
Thanks Jack!

[jackkoenig]

Fixed by #2286, to be released in FIRRTL 1.4.4 (Chisel 3.4.4)

[carlosedp]

I tested this out with Chisel3 3.5-SNAPSHOT as of 20/07/2021 and saw that if the memory instances have different width or size and different hex files, both instances are correctly generated in the output Verilog.

But I still see some problems with some cases:

• If I have both memory instances with same width, size and hex file, it gets deduplicated and only one module is generated in the output.
• If I have the memories with same width and size but different hex files, I get the error below:

[error] (run-main-e) firrtl.FirrtlUserException: At least one memory annotation did not deduplicate: got non-local annotation $a from [[DedupAnnotationsTransform]]
[error] firrtl.FirrtlUserException: At least one memory annotation did not deduplicate: got non-local annotation $a from [[DedupAnnotationsTransform]]
[error] stack trace is suppressed; run last Compile / bgRun for the full output
[error] Nonzero exit code: 1
[error] (Compile / run) Nonzero exit code: 1
[error] Total time: 2 s, completed Jul 20, 2021, 3:18:23 PM

My main use-case for this feature is to have reusable memory modules that could be customized in the generation, like one 32k memory A that is initialized by A.hex and a second 32k memory B that is initialized by B.hex.

Cc. @jared-barocsi

[jackkoenig]

@carlosedp looking at the problems you mention:

If I have both memory instances with same width, size and hex file, it gets deduplicated and only one module is generated in the output.

Is that a problem? They're identical in every way, are you wanting that to prevent deduplication?

If I have the memories with same width and size but different hex files, I get the error below:

Am I correct that the desired behavior is that there will be two modules, one initialized with each hex file?

[carlosedp]

@carlosedp looking at the problems you mention:

If I have both memory instances with same width, size and hex file, it gets deduplicated and only one module is generated in the output.

Is that a problem? They're identical in every way, are you wanting that to prevent deduplication?

This is correct, since they are the same about all characteristics, they should be de-duplicated. Sorry if I implied they shouldn't.

If I have the memories with same width and size but different hex files, I get the error below:

Am I correct that the desired behavior is that there will be two modules, one initialized with each hex file?

This is the problem I meant, they are the same object but have different initialization so shouldn't be deduplicated or give an error. Just both generated similar to the "different width/size" case with it's own file.

[jackkoenig]

This is the problem I meant, they are the same object but have different initialization so shouldn't be deduplicated or give an error. Just both generated similar to the "different width/size" case with its own file.

Okay this makes sense. Yeah I think we selected erroring because we weren't certain what to do, but you are right that it shouldn't be an error, the modules just shouldn't be deduplicated.

[jackkoenig]

I'm reopening this because while it's "mostly fixed" (and the remaining issue can be worked around at least), we should be duplicating instead of erroring.

[jackkoenig]

To workaround the error @carlosedp is seeing, you can make sure the modules instantiating the memories don't deduplicate. There are a few ways you can do this, trading off how explicit the workaround is vs. how well it works for other cases.

1. Use doNotDedup (https://www.chisel-lang.org/api/latest/chisel3/experimental/doNotDedup$.html). This is very explicit but relies on you as the user to determine when you want/need it vs. when you don't.
2. Add a "dedup blocking" constant Wire that will get optimized away but could prevent dedup. Since the relevant factor here is that you have differently named hex files but everything else is the same, I'd just take the hashcode of the name of the hex file and assign that to a wire in the module val dedupBlock = WireInit(memFileName.hashCode.U). This will only block dedup when the mem file names differ.

[carlosedp]

Thanks Jack, the second option is neat that I can add to the module object itself.
Will keep an eye here until the fix is sent.