Add VectorFIFOF module

[darius-bluespec]

The VectorFIFOF module has a FIFOF interface for enqueuing and dequeuing data. It also provides a method, which returns a Vector of Maybe entries, to access all entries in the queue simultaneously.

[quark17]

Can you add a header comment to this file (with copyright and license info), similar to what's in Semi_FIFOF.bsv, for example?

Also, the rules need to have the attribute fire_when_enabled on them, and I would also want no_implicit_conditions, to guarantee that the rules never fail to fire. This second attribute might fail for rl_enq, which can be resolved by changing the mkWire (which has a condition) to mkRWire (which doesn't).

I also think it'd be good to put a comment on the module saying what the method scheduling relationships are (or rather, what you expect them to be). I think the use of wires means that enq, deq, clear, and vector can all come in any order, so I'd like to document what you expect it to be. Personally, I think this is too free -- I'd like to see vector have to be before the others, because the vector value becomes stale after enqueuing, dequeuing, or clearing. I also think that clear should be after enq and deq. I think the one freedom that this FIFO allows is for enq and deq to be in either order (because you're not making one depend on the other, their conditions both have to be true at the start of the cycle, and so you can't enqueue into a full FIFO when also dequeuing or dequeue from an empty FIFO when also enqueuing, for example) -- that's worth calling out in a comment on the module, as it's where FIFO variants typically differ in their scheduling. If you're able to make the scheduling less free, that'd be wonderful, but I don't want to make more work for you if it's trouble, so just a comment on the state of things would be sufficient.

With those three changes, I'd be willing to merge it. However, I'd really prefer that the design not use wires, and put everything in the method actions -- both for formal cleanliness and to avoid unintentional freedoms in the method scheduling. But I don't want to make too much work for you (and this contrib repo can be a little loose), so as long as there are comments and attributes, I'm OK accepting it.

[darius-bluespec]

I pushed an updated branch that I believe addresses all of the
requested changes. The module has been restructured to eliminate
wires and rules, and documentation about the expected behavior has
been added.

[quark17]

Thank you! Although, I wasn't expecting such a dramatic change to the module -- are you OK with the current form (power of two requirement, vector isn't in order)? (I hadn't realized that the earlier version was shifting the elements on dequeue. I assume that either architecture is valid, but maybe the new one is better? Although I don't have a sense of how big the logic is for computing the valid bit for the vector elements in this version.)

Was there a reason you chose to use staticAssert? That check isn't performed unless the user gives a flag on the command line. The check for power of two should not be optional, so I would rewrite it to something like the following:

if (valueOf(depth) != valueOf(TExp#(TLog#(depth))))
  errorM("mkVectorFIFOF depth must be a power of 2");

// Or
if (valueOf(depth) != 2**log2(valueOf(depth)))
  errorM("mkVectorFIFOF depth must be a power of 2");

Or arguably make it a proviso, so that it is detected earlier:

provisos (
  Log#(depth, logdepth),
  Exp#(logdepth, depth)
);

// Or
provisos (
  NumAlias#(depth, TExp#(TLog#(depth)))  
);

The comment says that clear conflicts with enq and deq, which is true for parallel use (same action), but isn't true for sequential use (in separate rules). Because clear is writing a constant to the registers, it is able to order afterward, and shadows the earlier writes, so the user gets warnings about this shadowing. To avoid the shadow warnings, you can instantiate the head and tail registers as mkCReg and have the clear action write to the second port -- making it explicit to BSC that you want that order. (It'd be nice if the user could just write an execution_order attribute to specify this order, but BSC currently doesn't handle scheduling attributes on methods at non-synthesis boundaries.)

The comment also says that enq and deq can be called in either order, but that's also unfortunately not true -- they conflict. That means you can't enqueue and dequeue in the same clock cycle, which I'm sure is not what you want.

You can see the scheduling by synthesizing the module with a wrapper like this:

(* synthesize *)
module mkVF(VectorFIFOF#(4,Bool));
  (* hide *)
  VectorFIFOF#(4,Bool) __i <- mkVectorFIFOF;
  return __i;
endmodule

and then if you run BSC with the flag -show-method-bvi, it will put the scheduling relationships in a comment in the generated Verilog file:

// BVI format method schedule info:
// schedule fifo_enq  SBR ( fifo_clear );
// schedule fifo_enq  C ( fifo_enq, fifo_deq );
//
// schedule fifo_deq  SBR ( fifo_clear );
// schedule fifo_deq  C ( fifo_enq, fifo_deq );
//
// schedule fifo_first  CF ( fifo_first, fifo_notFull, fifo_notEmpty, vector );
// schedule fifo_first  SB ( fifo_enq, fifo_deq, fifo_clear );
//
// schedule fifo_notFull  CF ( fifo_first, fifo_notFull, fifo_notEmpty, vector );
// schedule fifo_notFull  SB ( fifo_enq, fifo_deq, fifo_clear );
//
// schedule fifo_notEmpty  CF ( fifo_first,
//                           fifo_notFull,
//                           fifo_notEmpty,
//                           vector );
// schedule fifo_notEmpty  SB ( fifo_enq, fifo_deq, fifo_clear );
//
// schedule fifo_clear  SBR ( fifo_clear );
//
// schedule vector  CF ( fifo_first, fifo_notFull, fifo_notEmpty, vector );
// schedule vector  SB ( fifo_enq, fifo_deq, fifo_clear );

The computation of notFull and notEmpty uses both registers, which is why enq and deq conflict. It also means that the notFull and notEmpty methods have to be used before enq and deq. And first has to be before enq, so even if deq could come after, a rule that used both deq and first could not. Also note that vector has to come before enq and deq; if you're using this in a design that checks vector before deciding to enq, then doing that in one rule would have to come before a rule that does deq (and so deq would in practice only be possible after enq, or else you have to split up the rules in your design into separate pieces, communicating by wires, which would be less error-prone to do inside the FIFO).

[darius-bluespec]

Thanks for all the feedback and insight. I pushed another attempt at
this. I went back to an architecture more similar to my original
design. I think my last changes were trying too hard to keep all
behavior in the methods. The current design should result in somewhat
better generated hardware, with respect to muxing on the data output.
I also looked at some existing FIFOs and took some inspiration from
them. The schedule looks much improved to me, at perhaps the expense
of somewhat more complicated module.

[quark17]

The schedule is back to being extremely permissive. For example, the clear method is allowed to sequence before the enq and deq methods, but the behavior inside the module is that the clear method happens last: if both enq and clear are asserted, the count is 0, not 1. The permissive schedule on the FIFO allows an enqueuing rule to execute after a clearing rule; if those rules are also keeping state relative to each other, then it will become out of sync with the FIFO state.

I'm not opposed to accepting the current design (if the comment is improved to clarify the module's behaviors and where the schedule is more permissive) but I'd suggest tightening the schedule, if you're OK with that?

If nothing else, you could insert uses of mkRevertingVirtualReg in the way that's used by some of the modules in SpecialFIFOs. For example:

import RevertingVirtualReg ::*;
...

Reg#(Bool) beforeEnq <- mkRevertingVirtualReg(True);
Reg#(Bool) beforeDeq <- mkRevertingVirtualReg(True);
Reg#(Bool) beforeClear <- mkRevertingVirtualReg(True);

Bool beforeActions = beforeEnq && beforeDeq && beforeClear;

method Action clear(t x);
  ...
  beforeClear <= False;
endmethod

method Action enq(t x) if (beforeClr);
  ...
  beforeEnq <= False;
endmethod

method Action deq(t x) if (beforeClr);
  ...
  beforeDeq <= False;
endmethod

method Bool notEmpty() if (beforeActions) = ...;

method Bool notFull() if (beforeActions) = ...;

method t first () if (beforeDeq && beforeClear) = ...;

method vector() if (beforeActions) = ...;

(Note that vector isn't actually an interface, it's just a value method. The BSV parser will allow you to declare it either way, since they're both just functions in the internal representation, but you might want to declare it as a method in the code, to be correct.)

I've illustrated the scheduling that I think is important here (following the explicit schedule given for the import of FIFO2 as vMk2FIFOF in FIFOF_.bsv):

• enq and deq come before clear
• notFull and notEmpty come before all of the actions (because the actions can change the values)
• first comes before deq and clear
• vector comes before all of the actions (like notFull and notEmpty, it is only valid at the start of the cycle)

The mkRevertingVirtualReg module has no actual state: the D_IN is ignored and the Q_OUT is constant. Unfortunately, BSC's reg/wire inlining doesn't currently inline them, so you will get a module instantiation. (That's something we could improve, though.) There may be a way to add scheduling relationships without them (by moving some of the state out of the magic rule), but that would require some more thought; using this module doesn't require changing anything else.

[quark17]

If you're OK with these changes, I can push them to the PR (rather than you having to do it), since I've already made the changes locally. I can also add a test of the schedule to the testsuite.

[darius-bluespec]

Those changes sound fine to me. Feel free to use what you have
working.