[libre-riscv-dev] pipeline sync issues

[Luke Kenneth Casson Leighton]

On Friday, April 12, 2019, Jacob Lifshay <programmerjake at gmail.com> wrote:

> On Thu, Apr 11, 2019 at 11:43 PM Luke Kenneth Casson Leighton <
> lkcl at lkcl.net>
> wrote:
>
> > the advantage of this approach is that if it's determined that there
> > is too great a gate delay, all that's needed is: split those
> > daisy-chains into shorter ones, throw the shorter bits at another
> > derivative of ControlBase, and the overall pipeline goes up by one
> > cycle...
> >
> > .... *with very little coding effort*.
> >
> > unfortunately, it's not quite that simple (that's what pipeline.py is
> > supposed to be for), it's necessary to determine and change the
> > ispec/ospec, as part of any given (hypothetical) split /
> > extension-to-overall-pipeline-length.
> >
> > pipeline.py is supposed to hide even that, transparently, by
> > dynamically working out the ospec and ispec depending on the runtime
> > connections.
> >
> In the original code I had written, with the addition of a *Chain class,
> that could have been done quite simply by adding a new instance of RegStage
> in the middle of the chain (basically just calling a constructor). no other
> code needs to be changed.


This requires stage conceptually to be both a data processing block and a
control handling block.
Which still makes me nervous due to the lack of separation.

Now that I think about it, there *may* be a way to resolve this through the
addition of a new class type that does exactly that, and allows it to be
"embedded" within a Control Block.

If you look at FIFOControl you can see how, in effect, a data/ready/valid
in/out thing is "wrapped" inside a Control Block.

This is doing my head in, to be honest :)


> >
> >
> > > In my proposal, I'm thinking of switching the Stage/Pipeline naming to
> a
> > > new naming scheme, to avoid confusion coming from semantics already
> > > attached to those names.
> >
> >  yes.  i started switching to "Control Handling" and "Data Processing"
> > a few days ago.  i left in the name "Stage" unfortunately.
> >
> Ok. Since my proposal doesn't have the same control vs. data split, I'll
> probably use "block" as it has no conflicting meanings in the context of
> pipelines.


The sentence that connects them together is, "it is only when a Control
Block is given with a Data Processing Block to manage that a pipeline stage
is created".


>
>  it had occurred to me that there may be time-related [unnecessary]
> > delays associated with BreakReadyChainStage which mean that it works
> > yet is sub-obtimal.  dan gisselquist warned of this as possibility
> > that is common in many efforts to design valid/ready (WE/WRITEABLE +
> > RE/READABLE) pipeline signalling.
> >
> Ok. BreakReadyChainStage is designed to be used sparingly, most simple
> pipelines would be made from RegStage and CombStage. If we chose a variant
> of RegStage that doesn't fill empty slots in a pipeline when later slots
> are stalled, then ready is wired directly from input to output in both the
> RegStage variant and CombStage.


Until the reason for the bug where all of the variants that you created do
not work when connected after BufferedHandshake, we cannot use any of them.
BreakReadyStage, RegStage, *or* UnbufferedPipeline.

I've removed all uses of UnbufferedPipeline from FPADD as a result.

It may require some formal proofs to track this down.

Until that bug is tracked down, my biggest concern is that the proposal
that you are writing is based on a serious logic flaw that could result in
significant work taking place based on a fundamentally flawed design:
months of work take place only to have to undergo a major rewrite.

Or worse, USD 100k is spent on a MPW run that doesn't work.


I am reaaasonably confident with the new FIFOControl as SyncFIFO has gone
through formal correctness checking, and likewise SimpleHandshake as it is
based on proven logic deployed in wishbone and AXI4.

However, annoyingly, I am beginning to appreciate that without formal
proofs we really cannot just rely on that.

In addition, what has me concerned is that when dropping in
SyncFIFOBuffered in place of SyncFIFO the unit tests FAIL. This will also
need to be tracked down, and my main concern is that the logic in
SyncFIFOBuffered does not have a formal correctness proof, so the bug may
actually be there, not in the unit test.

This is really quite complicated and well beyond what I was expecting to
spend several weeks on!

L.



-- 
---
crowd-funded eco-conscious hardware: https://www.crowdsupply.com/eoma68