schoeberl / chisel-book

Digital Design with Chisel
752 stars 141 forks source link

Possible bug in MemFifo in fifo.scala #15

Closed mmoghad closed 1 year ago

mmoghad commented 4 years ago

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.

schoeberl commented 4 years ago

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

sterin commented 3 years ago

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)
      }
    }
  }
}
schoeberl commented 1 year ago

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

schoeberl commented 1 year ago

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