chipsalliance / firrtl

Flexible Intermediate Representation for RTL
https://www.chisel-lang.org/firrtl/
Apache License 2.0
731 stars 177 forks source link

read port inference: enable should be true not false #1998

Open ekiwi opened 3 years ago

ekiwi commented 3 years ago

What is the current behavior?

I have this Chirrtl file which is a simplified version of one of my Chisel designs:

;buildInfoPackage: chisel3, version: 3.4.0, scalaVersion: 2.12.12, sbtVersion: 1.3.10
circuit WishBoneRam : 
  module WishBoneRam : 
    input clock : Clock
    input reset : UInt<1>
    input io : {adr : UInt<8>, flip rdt : UInt<32>}

    smem mem_0 : UInt<8>[64], undefined @[WishBoneRam.scala 16:36]
    smem mem_1 : UInt<8>[64], undefined @[WishBoneRam.scala 16:36]
    smem mem_2 : UInt<8>[64], undefined @[WishBoneRam.scala 16:36]
    smem mem_3 : UInt<8>[64], undefined @[WishBoneRam.scala 16:36]
    node wordAlignedAddress = bits(io.adr, 7, 2) @[WishBoneRam.scala 17:34]
    read mport io_rdt_right_right = mem_0[wordAlignedAddress], clock @[WishBoneRam.scala 23:31]
    read mport io_rdt_right_left = mem_1[wordAlignedAddress], clock @[WishBoneRam.scala 23:31]
    read mport io_rdt_left_right = mem_2[wordAlignedAddress], clock @[WishBoneRam.scala 23:31]
    read mport io_rdt_left_left = mem_3[wordAlignedAddress], clock @[WishBoneRam.scala 23:31]
    node io_rdt_left = cat(io_rdt_left_right, io_rdt_left_left) @[Cat.scala 29:58]
    node io_rdt_right = cat(io_rdt_right_right, io_rdt_right_left) @[Cat.scala 29:58]
    node _io_rdt_T = cat(io_rdt_right, io_rdt_left) @[Cat.scala 29:58]
    io.rdt <= _io_rdt_T @[WishBoneRam.scala 23:10]

Somehow the read port inference fails for all but the last read port. The read ports for memory 0-2 all get their enabled field set to UInt(0) which results in their address being stuck at zero in the final Verilog output.

After RemoveChirrtl:

circuit WishBoneRam :
  module WishBoneRam :
    input clock : Clock
    input reset : UInt<1>
    input io : { adr : UInt<8>, flip rdt : UInt<32>}

    mem mem_0 : @[WishBoneRam.scala 16:36]
      data-type => UInt<8>
      depth => 64
      read-latency => 1
      write-latency => 1
      reader => io_rdt_right_right
      read-under-write => undefined
    mem_0.io_rdt_right_right.addr is invalid @[WishBoneRam.scala 16:36]
    mem_0.io_rdt_right_right.clk is invalid @[WishBoneRam.scala 16:36]
    mem_0.io_rdt_right_right.en <= UInt<1>("h0") @[WishBoneRam.scala 16:36]
    mem mem_1 : @[WishBoneRam.scala 16:36]
      data-type => UInt<8>
      depth => 64
      read-latency => 1
      write-latency => 1
      reader => io_rdt_right_left
      read-under-write => undefined
    mem_1.io_rdt_right_left.addr is invalid @[WishBoneRam.scala 16:36]
    mem_1.io_rdt_right_left.clk is invalid @[WishBoneRam.scala 16:36]
    mem_1.io_rdt_right_left.en <= UInt<1>("h0") @[WishBoneRam.scala 16:36]
    mem mem_2 : @[WishBoneRam.scala 16:36]
      data-type => UInt<8>
      depth => 64
      read-latency => 1
      write-latency => 1
      reader => io_rdt_left_right
      read-under-write => undefined
    mem_2.io_rdt_left_right.addr is invalid @[WishBoneRam.scala 16:36]
    mem_2.io_rdt_left_right.clk is invalid @[WishBoneRam.scala 16:36]
    mem_2.io_rdt_left_right.en <= UInt<1>("h0") @[WishBoneRam.scala 16:36]
    mem mem_3 : @[WishBoneRam.scala 16:36]
      data-type => UInt<8>
      depth => 64
      read-latency => 1
      write-latency => 1
      reader => io_rdt_left_left
      read-under-write => undefined
    mem_3.io_rdt_left_left.addr is invalid @[WishBoneRam.scala 16:36]
    mem_3.io_rdt_left_left.clk is invalid @[WishBoneRam.scala 16:36]
    mem_3.io_rdt_left_left.en <= UInt<1>("h0") @[WishBoneRam.scala 16:36]
    node wordAlignedAddress = bits(io.adr, 7, 2) @[WishBoneRam.scala 17:34]
    mem_3.io_rdt_left_left.en <= UInt<1>("h1") @[WishBoneRam.scala 17:34]
    mem_0.io_rdt_right_right.addr <= wordAlignedAddress @[WishBoneRam.scala 23:31]
    mem_0.io_rdt_right_right.clk <= clock @[WishBoneRam.scala 23:31]
    mem_1.io_rdt_right_left.addr <= wordAlignedAddress @[WishBoneRam.scala 23:31]
    mem_1.io_rdt_right_left.clk <= clock @[WishBoneRam.scala 23:31]
    mem_2.io_rdt_left_right.addr <= wordAlignedAddress @[WishBoneRam.scala 23:31]
    mem_2.io_rdt_left_right.clk <= clock @[WishBoneRam.scala 23:31]
    mem_3.io_rdt_left_left.addr <= wordAlignedAddress @[WishBoneRam.scala 23:31]
    mem_3.io_rdt_left_left.clk <= clock @[WishBoneRam.scala 23:31]
    node io_rdt_left = cat(mem_2.io_rdt_left_right.data, mem_3.io_rdt_left_left.data) @[Cat.scala 29:58]
    node io_rdt_right = cat(mem_0.io_rdt_right_right.data, mem_1.io_rdt_right_left.data) @[Cat.scala 29:58]
    node _io_rdt_T = cat(io_rdt_right, io_rdt_left) @[Cat.scala 29:58]
    io.rdt <= _io_rdt_T @[WishBoneRam.scala 23:10]

What is the expected behavior?

.en should be UInt(1) for all four read ports.

@albert-magyar : is this a known bug?

ekiwi commented 3 years ago

Update

In my Chisel source, changing:

io.rdt := Cat(mem.map(_.read(wordAlignedAddress)))

to

io.rdt := Cat(mem.map(_.read(wordAlignedAddress, true.B)))

seems to fix the problem.

albert-magyar commented 3 years ago

This is the same issue as #840 and #913. TLDR: the mport interface effectively has no spec aside from how the current implementation works, and many patterns that use non-wires as addresses do not work.

The current reliable workaround is to use store the address in an intermediate wire.