Eliminate CHIRRTL

[azidar]

• Type of issue

  • Bug report
  • Feature request
  • Other enhancement

• What is the use case for changing the behavior?
  Lowering pass generates Firrtl with memory's clock connected to validIf #702 exposes a problem with the current way we are handling memory ports with CHIRRTL. The plan of record is to remove CHIRRTL entirely, and add MPort as first-class node in the FIRRTL IR.

This issue is to track discussions about how to make this possible.

• Impact

  • no functional change
  • API addition (no impact on existing code)
  • API modification
  • unknown

• Development Phase

  • request
  • proposal

The advantages of this approach is that CHIRRTL is a wart, bridging how FIRRTL handles memories, and how Chisel handles memories. See #702 for more detail.

[azidar]

The current proposal is to remove MPort's after running ExpandWhens. To handle mport enables, we will need to add an explicit enable field in MPorts.

One problem I haven't solved yet is how to handle the read enable for non-combinational read ports (for combinational reads, the enable can always be true without loss of QOR). The current CHIRRTL and Chisel semantics is that the enable is true wherever the address is combinationally assigned to (thus always true if address is a register), not where it is declared. Thus:

mem m:
  readLatency => 1
  ...
read mport r = m[addr], clk
when p:
  out <= r

should be compiled to:

mem m:
  readLatency => 1
  reader => r
m.r.en <= p
m.r.addr <= addr
out <= validif(p, m.r.data)

This would just require more special casing in ExpandWhen's to keep track of wherever these mports are referenced, and then add them to the enable field. If feels very inelegant.

[azidar]

Another thought is to change mports to have two fields, an enable and a data field:

mem m:
  readLatency => 1
  ...
read mport r = m[addr], clk
r.en <= UInt(0)
when p:
  out <= r.data
  r.en <= UInt(1)

should be easily expanded after ExpandWhens:

mem m:
  readLatency => 1
read mport r = m[addr], clk
r.en <= mux(p, UInt(1), UInt(0))
out <= validif(p, r.data)

then the mports can be removed:

mem m:
  readLatency => 1
  reader => r
m.r.en <= mux(p, UInt(1), UInt(0))
m.r.addr <= addr
out <= validif(p, m.r.data)

[albert-magyar]

@azidar, one thing to note is that RemoveCHIRRTL currently defines that the enable is true whenever the address used for the read port is assigned -- from looking at the code, this seems like it was an intentional move, not an implementation bug.

I have a patch for RemoveCHIRRTL that actually makes the semantics line up with what you're suggesting (in the when contexts where the read port is referenced) and also fixes the bugs encountered in #286, #512, #840, #913, and #1161, but unfortunately it is also kind of an API change, due to the fact that the current enable rules are more-or-less "specified" by the pass behavior.

[azidar]

Plan of record is to support memory port as new IR statement to support, which has a clock and enable port. Have to remove after expandwhens but before middle form, is part of high form.

CHIRRTL:

mem m:
  readLatency => 1
  ...
when p:
  wire addr: UInt
  addr <= io.addr
  read mport r = m[addr], clk
  out <= r

AFTER RESOLVE ENABLE:

mem m:
  readLatency => 1
  ...
when p:
  wire addr: UInt
  addr <= io.addr
  wire enable: UInt
  enable <= UInt(1)
  read mport r = m[addr], clk, enable
  out <= r

AFTER EXPAND WHENS:

mem m:
  readLatency => 1
  ...
wire addr: UInt
addr <= io.addr
wire enable: UInt
enable <= UInt(1)
read mport r = m[addr], clk, enable
out <= validif(p, r)

AFTER REMOVE MPORT:

mem m:
  readLatency => 1
  ...
wire addr: UInt
addr <= io.addr
wire enable: UInt
enable <= UInt(1)
m.r.addr <= addr
m.r.clk <= clk
m.r.en <= enable
out <= validif(p, r)

Why you can't support unconditional clock assignment to mem port:

mem m:
  readLatency => 1
  ...
m.r.clk <= ???
when p:
  wire addr: UInt
  addr <= io.addr
  wire clock: Clock
  clock <= asClock(x)
  read mport r = m[addr], clock
  out <= r

[albert-magyar]

Adding on some more summary notes for future reference:

• Spec should define reads as unconditionally enabled when registers / nodes / ports are used for addresses
• Handling of enable signals must avoid generating forward references in code
• The addition of an enable field to the internal representation of an mport makes some cases easier to support. It can also be inferred from input FIRRTL that doesn't use such a field.

Since this subsumes #286, #512, #840, #913, and #1161, I've closed those issues.

[azidar]

New conclusion: add optional enable to mport. resolve whens. with flag, add optimization that deasserts enable when address is invalid. This preserves existing behavior, gives clear spec semantics, and has a clean implementation.

[aswaterman]

Sounds reasonable, but the enable pin often has a long setup time (moreso than the address for some RAMs), so any magic that could add additional logic to the enable should be optional. In particular, it shouldn't be mandatory that an invalid address kills the enable, even though that's a valid thing to do.

[albert-magyar]

Sounds reasonable, but the enable pin often has a long setup time (moreso than the address for some RAMs), so any magic that could add additional logic to the enable should be optional. In particular, it shouldn't be mandatory that an invalid address kills the enable, even though that's a valid thing to do.

That is actually well-aligned with how we're changing it. The current implementation of RemoveCHIRRTL (as of primordial times) actually already sets the read-enable to the valid condition of the address for all memories in every design.

What we've now resolved is that there will be a new explicit enable that is driven by a new, optional parameter in the Chisel API. The spec for inferred mports without an explicit enable (including all current designs) will be that they are always enabled. An optional optimization pass could then disable the read in cases where the address is invalid if aggressive readwrite inference is desired for memories without explicit enables. Currently, this optimization is effectively part of the "implicit spec" of CHIRRTL, but we're choosing to leave this enable-on-address-valid behavior out of the spec for inferred memory ports because a) it might be confusing and b) it might not be the microarchitecture the user actually wants.

[aswaterman]

Thanks for the explanation. It seems like a pretty coherent strategy (just don't mess up code gen for current designs in the process of implementing it :))