mkSizedBypassFIFOF(1) doesn't behave the same as mkBypassFIFOF

[qschibler]

Using this code inside a top module :

Reg#(int) counter <- mkReg(0);

rule count;
  if(4 <= counter)
    $finish;

  counter <= counter + 1;
endrule

FIFOF#(int) fifo <- mkBypassFIFOF();

rule enq if(fifo.notFull);
  $display("%0t enq %h", $time, counter);
  fifo.enq(0);
endrule

rule deq if(fifo.notEmpty);
  $display("%0t deq %h", $time, counter);
  fifo.deq();
endrule

I get the following expected result :

10 enq 00000000
10 deq 00000000
20 enq 00000001
20 deq 00000001
30 enq 00000002
30 deq 00000002
40 enq 00000003
40 deq 00000003
50 enq 00000004

But if I change the fifo to a mkSizedBypassFIFOF(1), I get the following output :

10 enq 00000000
20 deq 00000000
30 enq 00000002
40 deq 00000002
50 enq 00000004

It looks like our fifo does not bypass anymore...

Looking quickly at the source code, It seems that mkSizedBypassFIFOF(1) is implemented using a C function/verilog file, whereas mkBypassFIFOF is implemented using a CReg.

Also to avoid shifts in behaviour, maybe we should implement mkBypassFIFOF as just returning a mkSizedBypassFIFOF(1) instance ?

[quark17]

I am unable to reproduce this issue. When I change to use mkSizedBypassFIFOF(1), the output is the same. Can you provide the complete file and the command line used to compile it, so that I can try reproducing exactly what you're doing?

As for combining the variations to use the same codebase: That's certainly something worth doing. The SpecialFIFOs library grew a organically and ad hoc. See one of my comments on #446 for some history. There is a similar issue with the mk*PipelineFIFO* modules, where we further have duplicates of the same behavior under the older name of mk*LFIFO*. The LFIFO modules are imported primitives while the PipelineFIFO modules are written in BSV. For viewing VCDs and debugging behavior of a design, the BSV-written modules get inlined into the parent module and you lose some convenience of being able to probe the methods of the FIFO; so I prefer the primitives or separately synthesized modules, when debugging. For quality of synthesis, I'd heard from a user in 2017 that they were seeing a variation in the number of LUTs (when synthesizing for FPGA) between mkSizedBypassFIFO(1) and mkBypassFIFO -- but for some designs one was better and for other designs the other was better. Both synthesis tools and BSC have improved since then, so I don't know what the current situation is; but it'd be worth knowing before deciding to eliminate one of the definitions, or maybe is a reason to keep both around in some form.

[quark17]

Also, just to be safe, include the version of BSC that you're using (as indicated by bsc -v).

[qschibler]

Here is the version output : Bluespec Compiler, version 2022.01 (build 066c7a87e)

And here are the two commands I use to build, where I replaced my project directory by DIR: bsc -sim -bdir DIR/bin -simdir DIR/bin -vdir DIR/bin -suppress-warnings G0023:G0020:S0080:S0089 -Xc++ -Wno-format-overflow -Xc++ -Wno-dangling-else -Xc -Wno-stringop-truncation DIR/src/Tb.bsv

bsc -sim -bdir DIR/bin -simdir DIR/bin -vdir DIR/bin -suppress-warnings G0023:G0020:S0080:S0089 -Xc++ -Wno-format-overflow -Xc++ -Wno-dangling-else -Xc -Wno-stringop-truncation -e mkTb -o DIR/bin/exe

[quark17]

I just noticed that you are explicitly calling fifo.notFull and fifo.notEmpty in your rules. You do not need to do this, because the enq and deq methods have implicit conditions that will check for this. And in fact, for the version of BSC that you're using, the notEmpty method on mkSizedBypassFIFOF is defined as the state of the FIFO at the start of the clock cycle, not the state of the FIFO after enq has potentially been called (which is how the implicit condition of deq is defined), and so your design is working as it's written! This definition for notEmpty may not be what users expect, so it was raised as issue #446. As explained in a comment there, the notEmpty method can have three different definitions (and it might be worth having a version of the FIFO that provides all three), but since the FIFOF interface has only one notEmpty method, it probably makes sense for that method to match the implicit condition on the deq method. So this change was made, after the version of BSC that you're using.

In case that wasn't clear, I can demonstrate with pseudocode. You've written this:

rule deq if (fifo.notEmpty);

but BSC will look inside the body of the rule and find the actions that have implicit conditions and it will add them to the condition of the rule, so inside the compiler the rule will look like this:

rule deq if (fifo.notEmpty && condition(fifo.deq));

The confition of fifo.deq is that the FIFO is not empty, at the moment when the rule is being considered. If the FIFO is empty at the start of the clock cycle, then the condition at that time is False; but if a rule enqueues into the FIFO, then the condition becomes True; and then if you then dequeue, the FIFO becomes empty again (the condition is False). The wires for the notEmpty method can only provide one of those values. But you can imagine having methods notEmpty1, notEmpty2, and notEmpty3, providing the values at each of those points in the schedule. With your version of BSC, the rule that you've written becomes this:

rule deq if (fifo.notEmpty1 && fifo.notEmpty2);

because the explicit method call uses notEmpty1 and the implicit condition of deq uses notEmpty2. This rule won't fire if the FIFO was empty at the start of the clock cycle, even if the FIFO becomes not empty after an enqueue. You can fix this by removing the explicit call to fifo.notEmpty, and your rule becomes just:

rule deq if (fifo.notEmpty2);

If you upgrade to a newer BSC, the definition for the explicit call has been changed to notEmpty2, and so your code is just being redundant and BSC will produce this:

rule deq if (fifo.notEmpty2 && fifo.notEmpty2);

This is redundant, but it works as you expect.

My suggestion would be to do both, remove the explicit calls and upgrade to a newer BSC. (There was also a minor schedule issue with mkSizedBypassFIFOF that was fixed at the same time as redefining the method.)