chisel-book icon indicating copy to clipboard operation
chisel-book copied to clipboard

Possible bug in MemFifo in fifo.scala

Open mmoghad opened this issue 4 years ago • 2 comments

HI,

I believe there is a bug in the MemFifo implementation in fifo.scala. It is shown the figure below. If the FIFO gets two consecutive words (while empty) during which it can produce output (deq.ready is high), then the second word will never be output. This is because the "valid" state will set emptyReg to true (will override writes decision since it comes after it in code), so back in Idle state, it will get stuck (since there are no more writes setting emptyReg to false.

Screenshot 2020-04-08 at 14 23 24

My solution is to put the write handling block (below), after the read handling switch statement. when (io.enq.valid && !fullReg) { mem.write(writePtr, io.enq.bits) emptyReg := false.B fullReg := nextWrite === readPtr incrWrite := true.B }

I haven't fully tested it but it seems to work now.

mmoghad avatar Apr 08 '20 12:04 mmoghad

Sorry for the late answer. Would it be possible to write a small test that shows the bug? And better place an issue into the original source in: https://github.com/freechipsproject/ip-contributions

schoeberl avatar Jul 15 '20 14:07 schoeberl

Hi,

I've also noticed the problem. Here's a simple test using ChiselTest:

class FifoSpec extends BaseSpec {

  "RegFIFO" in {
    test(new RegFifo(UInt(16.W), 16)) { 
      dut => {

        // First cycle: enqueue for one cycle, do not dequeue
        dut.io.deq.ready.poke(false.B)

        dut.io.enq.valid.poke(true.B)
        dut.io.enq.ready.expect(true.B)
        dut.clock.step(1)

        // Second cycle: enqueue and dequeue at once
        dut.io.deq.ready.poke(true.B)
        dut.io.deq.valid.expect(true.B)

        dut.io.enq.valid.poke(true.B)
        dut.io.enq.ready.expect(true.B)
        dut.clock.step(1)

        // Third cycle, the FIFO should contain a single element, but the ready signal remains low arbitrarily long

        dut.io.enq.valid.poke(false.B)
        dut.io.deq.ready.poke(true.B)

        dut.io.deq.valid.expect(true.B)
      }
    }
  }
}

sterin avatar Feb 01 '21 09:02 sterin

Thank you for the test! For some reason, I overlooked the notification. I am looking into this right now.

schoeberl avatar Nov 14 '22 02:11 schoeberl

The bug has been fixed in ip-contributions, and the code has been copied over into the book.

schoeberl avatar Nov 17 '22 23:11 schoeberl