[FIRRTL] Support multiple ports for smem

[SpriteOvO]

Fixes #4846.

[sequencer]

I think this is a temporary fix. Basically, The current way of memory instantiation is an anti-pattern in the most of ASIC designs. I think we need a new memory intrinsic in both chisel and CIRCT. The memory type should be predefined in RTL by designer, rather than the current behavior model to infer memory ports.

[seldridge]

This may just work. We'd need to check that this doesn't actually break anything as there may be assumptions in some of these passes that memories fit the restrictive SFC format for memories.

Basically, this should update testes and likely add new tests that check the behavior.

One thing about this: do we actually need some way of providing an option on which memories will be replaced? The requirements of which memory to replace are likely a function of the memory compiler you have access to. Do you have information on what parameters memory compilers will accept?

[sequencer]

Sorry for the late reply.

do we actually need some way of providing an option on which memories will be replaced?

I do believe we need an optional attribute to do this, at least in our ASIC deisgns, those memories are chosen before the RTL start.

The requirements of which memory to replace are likely a function of the memory compiler you have access to. Do you have information on what parameters memory compilers will accept?

Yes, we use TSMC MC2 for now, I'm drafting this chipsalliance/chisel#3131 for general SRAM intrinsic, which is the experience from MC2.

[SpriteOvO]

@seldridge Hi, I have fixed the test failures and added a new test case for the behavior, would you mind reviewing this PR again?

[seldridge]

I'm validating the following statement right now.

I think this is going to cause problems for existing flows which are relying on this not happening. Specifically, now any flow may see more SRAMs blackboxed than before. However, we clearly need a way to change this.

1. We can modify the existing FIRRTL memory (in CIRCT) to include information that it is a memory that should be replaced with a blackbox. (I don't want to use an annotation as that could become non-local which would be weird.)
2. We add a Chisel API that exposes FIRRTL memories more directly and adds the ability to use this attribute. This could be a CIRCT intrinsic.
3. We change places in the code where the heuristic is used to use either the heuristic or the memory attribute indicating replacement. If the memory heuristic would apply and the memory is not marked as being eligible for replacement, we print a warning telling users to go fix their code.
4. We remove the heuristic.

As a slight alternative, we could move away form FIRRTL memory here and switch to a CIRCT intrinsic explicitly. This may make the switchover more straightforward.

@darthscsi: do you have any thoughts?

[darthscsi]

There should be a way in chisel to specify the memories you want when you want specific things. It should be possible to control the lowering of memories from the flow. Which is to say, patters conditions should be options.

One question for non-behavioral memories (no inference) is whether there is any value in representing them at all. The only reason to have a specific representation is if we need to do something or know the semantics. If the user is placing specific memories and we just pass them through, how is that different from an external module?

[seldridge]

Given the above thoughts, we could switch directly to exposing a memory intrinsic and keeping that around until it becomes an external module. The lowering of the heuristic can then be preserved without breaking anything. @SpriteOvO: would your usage of this be amenable to using a different Chisel API for creating memories in your design or are you stuck using existing behavioral memories that cannot be changed?

[sequencer]

would your usage of this be amenable to using a different Chisel API for creating memories in your design or are you stuck using existing behavioral memories that cannot be changed?

Sorry for the late reply.

Basically, in our designs, the problem is #4846, which certainly is a bug.

If using other workarounds, we need to change the flow of our PnR design... I think if this PR does fix #4846, merging this will help us a lot.

[seldridge]

@richardxia: Do you have any idea of the consequences of this change in seq_mems.json/tb_seq_mems.json?

[richardxia]

I just took a quick look at our internal codebase, and I think this might actually be safe due to an idiosyncrasy of Python, which is that the integer 0 is considered "falsy" in boolean checks and all other integers are considered "truthy".

That said, if no outside of SiFive is using this file and we consider it to be deprecated, it we may want to err on the conservative side by preserving the original format and emitting a true if the number is greater than 0 and false otherwise.

[seldridge]

I think we can proceed with this while the better Chisel solution is sorted out. The reason why this is only handling 1rw, 1r, and 1r1rw is mostly arbitrary and this has been asked to be removed before: chipsalliance/firrtl#856

The one thing I see with this is that the mems.conf file isn't correct for more than two ports. I was playing around with this example:

circuit Qux:
  module Qux:
    input clock: Clock
    input r: {en: UInt<1>, flip data: UInt<8>, addr: UInt<1>}[2]
    input w: {en: UInt<1>, data: UInt<8>, addr: UInt<1>, mask: UInt<1>}[2]

    mem m :
      data-type => UInt<8>
      depth => 32
      reader => r0, r1
      writer => w0, w1
      read-latency => 1
      write-latency => 1
      read-under-write => undefined

    m.r0.clk <= clock
    m.r0.en <= r[0].en
    m.r0.addr <= r[0].addr
    r[0].data <= m.r0.data

    m.r1.clk <= clock
    m.r1.en <= r[1].en
    m.r1.addr <= r[1].addr
    r[1].data <= m.r1.data

    m.w0.clk <= clock
    m.w0.en <= w[0].en
    m.w0.data <= w[0].data
    m.w0.addr <= w[0].addr
    m.w0.mask <= w[0].mask

    m.w1.clk <= clock
    m.w1.en <= w[1].en
    m.w1.data <= w[1].data
    m.w1.addr <= w[1].addr
    m.w1.mask <= w[1].mask

And this was producing the following mems.conf file:

name m_ext depth 32 width 8 ports write,read

I think this should be:

name m_ext depth 32 width 8 ports write,write,read,read

However, I don't think that any tool that consumes this file has ever been tested for this.

[sequencer]

I think we should eventually use a better memory intrinsic to resolve this, for now, this PR may be a hotfix to downstream users with more than 1 ports.

[SpriteOvO]

circt/lib/Dialect/FIRRTL/Transforms/CreateSiFiveMetadata.cpp

Lines 105 to 108 in 1a67aff

	if (mem.getNumReadWritePorts() && isMasked)
	portStr = "mrw";
	else if (mem.getNumReadWritePorts())
	portStr = "rw";

Just noticed there was a mistake in the mainline, where = should be +=?

[SpriteOvO]

@seldridge Also, the mems.conf problem has been fixed in the force-pushed commit.

[seldridge]

LGTM

There is a possibility that this will break something internal to SiFive. I may have to back this out if that happens and we can figure out how to handle it.

[seldridge]

This is likely creating bugs when mixing write ports that are masked and unmasked: chipsalliance/chisel#3278