[FIRRTL] MemTap could be broken with aggregate preservation

[uenoku]

Currently registers of memories are emitted as unpacked arrays. MemTapAnnotation taps wires for each register but with aggregate preservation, these wires are emitted as packed arrays. Hence it will cause a complication error. For example,

circuit Top : %[[
  {
    "class": "sifive.enterprise.firrtl.MarkDUTAnnotation",
    "target":"~Top|DUTModule"
  },
  {
    "class":"firrtl.transforms.DontTouchAnnotation",
    "target":"~Top|Top>memTap"
  },
  {
    "class":"sifive.enterprise.grandcentral.MemTapAnnotation",
    "source":"~Top|DUTModule>rf",
    "sink":[
      "~Top|Top>memTap[0]",
      "~Top|Top>memTap[1]",
      "~Top|Top>memTap[2]",
      "~Top|Top>memTap[3]",
      "~Top|Top>memTap[4]",
      "~Top|Top>memTap[5]",
      "~Top|Top>memTap[6]",
      "~Top|Top>memTap[7]"
    ]
  }
]]
  module DUTModule :
    input clock : Clock
    input reset : Reset
    output io : { flip addr : UInt<3>, flip dataIn : UInt<8>, flip wen : UInt<1>, dataOut : UInt<8>}

    cmem rf : UInt<8> [8]
    infer mport read = rf[io.addr], clock
    io.dataOut <= read
    when io.wen :
      infer mport write = rf[io.addr], clock
      write <= io.dataIn

  module Top :
    input clock : Clock
    input reset : UInt<1>
    output io : { flip addr : UInt<3>, flip dataIn : UInt<8>, flip wen : UInt<1>, dataOut : UInt<8>}

    inst dut of DUTModule
    dut.clock <= clock
    dut.reset <= reset
    wire memTap : UInt<8>[8]
    memTap is invalid
    io.dataOut <= dut.io.dataOut
    dut.io.wen <= io.wen
    dut.io.dataIn <= io.dataIn
    dut.io.addr <= io.addr

Current output:

module rf_combMem(
...
  reg  [7:0] Memory[0:7];

----
module Top(
...
  wire [7:0][7:0] memTap;
  assign memTap = Top.dut.rf_ext.Memory;

It is not allowed to assign an unpaked array to a packed array.

[uenoku]

I added a workaround to lower ref type values for this issue https://github.com/llvm/circt/commit/8b105752d8d43881c9ad40d46827ee2adf75e551. Feel free to revert the commit.

To fix this issue, we have to lower reftype ports and tapped wires into unpacked arrays.

[dtzSiFive]

Discussed a bit, and the conclusion was that the issue is that cmem misrepresents its type as a vector (UInt<8>[8]), which in FIRRTL always means "packed".

Ref types only make it possible to avoid noticing it and generate invalid Verilog from (:disappointed: ; we could additionally eventually type-check XMR's in HW in the future?), but the heart of the matter is that it says it's a packed vector (and later, creates a ref type for a packed vector) and then its implementation is actually unpacked and that's no good.

I'm not sure how to best rework cmem to avoid this, but in CIRCT anyway could we perhaps just give it the right type (hw::UnpackedArray)? CHIRRTL is a distance from HW, but also it'd be good to not have it using an inaccurate type (and I don't believe we want to add an unpacked type to FIRRTL, at least not anytime soon?).