[libre-riscv-dev] [Bug 64] data handling / io control / data routing API needed

[bugzilla-daemon at libre-riscv.org]

http://bugs.libre-riscv.org/show_bug.cgi?id=64

--- Comment #3 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
Review of singlepipe.py
-----------------------

review of the following revision:

https://git.libre-riscv.org/?p=ieee754fpu.git;a=blob;f=src/add/singlepipe.py;h=fe052be72a3e70fde85e56058cf708ea9e7cd341;hb=25a0ec563bd7837b43a1d04036b2a5945c97023b

RecordObject
------------

not documented.  required due to Record not being able to modify fields.
should be moved to its own issue (#66).

PrevControl
-----------

self._o_ready and s_o_ready and use of i_valid_test not documented or
clear.  makes a bit of a mess of the code.

effort is designed to allow data processing blocks to have a degree
of control over when they accept incoming data (even if upstream
is otherwise ready)

really not clear.


NextControl
-----------

self.d_valid not documented or clear.

effort is designed to allow data processing blocks to control when
they *send* outgoing data (even if downstream is otherwise ready)

not clear.


Visitor
-------

a mess.  needs to be converted to an iterator/generator (yield)
needs __iter__ and __next__ (etc).

Eq
--

would be much simpler when Visitor is converted to a generator.


flatten
-------

needs to be converted to an iterator/generator, and for yield
part to be split out to a separate class/function (similar to
what has been done with eq/Eq/Visitor).


StageCls
--------

name is unclear.  postprocess experiment not documented.  


Stage
-----

name is again unclear.


RecordBasedStage
----------------

name ("stage") unclear.  purpose (actual need) is also unclear.


StageChain
----------

name ("stage") unclear.  need for specallocate parameter is not documented


ControlBase
-----------

* name "stage" not clear
* ControlBase allocating i_data / o_data not clear
* connect() function could (does?) overwrite i_data / o_data
* role of set_input not documented (or clear)
* recursive ports() function's purpose not clear (or documented)
* _elaborate rather than just elaborate, reason not clear
* "dynamic" ready/valid (involving stage in signalling) not clear
* addition of _postprocess not been discussed (or evaluated)


BufferedHandshake
-----------------

* known bugs (#57) in interaction with other control-blocks
* *may* be possible to replace with FIFOControl (see below)
* really complex and not obvious what's going on
* name not clear


SimpleHandshake
---------------

* based on wishbone / AXI4 signalling however actual characteristics unknown
* involved in bug #57
* name not clear


UnbufferedPipeline
------------------

* based on "RegStage" from proposal, however actual characteristics unknown
* involved in bug #57
* name not clear


UnbufferedPipeline2
-------------------

* based on "BreakReadyStage" from proposal, actual characteristics unknown
* involved in bug #57
* name not clear


PassThroughStage
----------------

* very useful as a building-block
* use of a single iospecfn however stops any kind of data width processing
  (this is good?)


PassThroughHandshake
--------------------

* name not clear
* based on "BreakReadyStage" however actual characteristics unknown
* is not involved in bug #57

RegisterPipeline
----------------

* probably should not be based on UnbufferedPipeline due to known issues #57
* better name needed?


FIFOControl
-----------

* nice class!
* buffered argument has to go (use new Queue)
* use of flatten() on output causes problems for stage postprocess
* comment about "looking like PrevControl" redundant due to removal of code
* i_data requiring a "shape" function is problematic.


Replacement classes
-------------------

all replacement classes need a proper audit and review of their own.
these are the classes with the exact same names, UnbufferedPipeline,
PassThroughHandshake, BufferedHandshake, SimpleHandshake.

should they even *be* replaced, due to the additional complexity
introduced through the use of Queue?  the graphviz layouts are a mess,
comparatively speaking.

-- 
You are receiving this mail because:
You are on the CC list for the bug.