[libre-riscv-dev] pipeline sync issues

[Luke Kenneth Casson Leighton]

On Wed, Apr 10, 2019 at 8:22 AM Jacob Lifshay <programmerjake at gmail.com> wrote:

> >  it also doesn't explain why UnbufferedPipeline (et al) *ignore*
> > p_o_ready entirely.  it's really important information as it
> > determines (or more like "declares") when the data is to *be* valid.
> >
> >  the point being: the truth table for UnbufferedPipeline has a case
> > where p_o_ready was *not* set, p_i_valid *is* set, and consequently
> > *invalid* data is sent on through the system.
>
> p_o_data should be an output set by the current stage.

 yes.

> if p_o_data is not
> set then the state of p_i_valid is a don't-care and no transfer logically
> occurs.

 no transfer should occur on *this* cycle... however the *current*
state of p_o_data is what the previous stage relies (relied) on in the
*previous* cycle, and consequently it is only when (p_o_ready ANDEd
with  p_i_valid) are set that the data is valid.

 that's where the confusion has occurred, and why the case exists
where the truth table mis-identifies data as invalid.

> >  * data and n_o_valid are sent *together* (to arrive on a future
> > cycle) and are only sent when n_i_ready is valid (in THIS cycle)
> >
> The way I had designed it was that n_o_valid is set and data is valid
> whenever the current stage has data that's ready to be sent, irrespective
> of if the next stage is ready yet, because it reduces the amount of logic
> necessary, though either way works.

 that's the thing: it doesn't work. and the two digraph diagrams (i
sent some screenshots earlier, and also put them in the bugreport) the
one that works has very little in the way of gate savings compared to
the one that does.

> I like to think of it from the perspective of the pipeline stages
> themselves rather than the perspective of the data flowing through, that
> way you don't have to keep track of any past, present, or future other than
> will the data be transferred at the next clock edge or not and making sure
> the data isn't deleted or duplicated by accident.

 that's... poossiblyyy why the confusion has arisen, as without taking
time into consideration, it's left that hole where invalid data got
through.

> > and if those two are not tested together, it *will* result in invalid
> > data being accepted.
> >
> yeah. keep in mind that since p_o_ready is an output that testing p_o_ready
> & p_i_valid may not have that exact expression in it.

 this confused the hell out of me as well.  why the heck would
p_o_ready be involved? moo? :)  it's because the state of p_o_ready
*now* is the state of p_o_ready *in the previous cycle*.

 and *that* information - part of the contract - is what needs to be
considered the "determiner-of-the-validity-of-the-data".

 considering only from the perspective of the pipeline stages, without
taking past-present-future time into consideration, is i believe what
led to the confusion.

> One possible way to fix the truth table is to write in each row how many
> data elements are stored in the stage and how many will be in the stage the
> next clock cycle and make sure that that matches. This is similar to the
> "transfer" column in
> https://salsa.debian.org/Kazan-team/simple-barrel-processor/blob/master/doc/BreakReadyChainStage.rst

 immediately, from the analysis i've done, i know that because it only
has 8 states, it's invalid.  8 states means that one of the 4 pieces
of information (i-v/r, o-v/r) is not being taken into account.

> Once the truth table is fixed then you just have to change the nmigen code
> to match.


> >
> > note: if we don't conform to the industry-standard practices for
> > ready/valid, we will not be able to use SyncFIFO in our designs.
> >
> I haven't checked the api, but building adaptors should be quite simple.

 ... it was.  w00t!

 https://git.libre-riscv.org/?p=ieee754fpu.git;a=blob;f=src/add/singlepipe.py;h=d7aee14d2dd3f4442def4c8449ee47dbc2539293;hb=fb8d2e66c4b263207d9990bdc542c46d6b37d48e#l919

i ran a couple of manual tests, setting the FIFO depth to 2 and to
4... and it works!  amazing.

l.