[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 #4 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
review of
https://salsa.debian.org/Kazan-team/simple-barrel-processor/blob/master/src/PipelineBuildingBlockAPIProposal.py

general comments
----------------

hard dependence on typing is causing readability issues and
will make contributions from experienced python programmers
that much harder.  *nobody* uses typing in common-usage python,
because it gets in the way.  there is an actual known paradigm
which python conforms to, that the use of typing violates
(i just can't recall what it is, right now).

basically python is based on the Liskov Substitution Principle,
of *semantic* relationships rather than *syntactical* relationships,
and introducing syntactical relationships increases code size,
decreases readability, decreases flexibility and breaks python
developer expectations.

https://en.wikipedia.org/wiki/Liskov_substitution_principle

can the typing be moved to a .pyi file and mypi deployed *if
the user wants to use it*?

Data, Shape, SignalShape, RecordShape, Empty, EmptyShape
----------.....

* addition of name extension (prefix) is a good idea.  if
  it can be achieved in the same way as nmigen, that would
  be even better.  nmigen uses this code (example):

  tracer.get_var_name(depth=2 + src_loc_at, default="$signal")

* restricting data usage using a syntactical hard limit instead
  of defining a semantic relationship results in limitations
  that a normal python developer will rebel against / reject.

  the limitation of *all* data using *forcibly* being required
  to go through the "Data" class instance is also a burden that
  will result in significant user demands for "additional features"
  on the Data paradigm, to add support for unanticipated
  requirements.

* the Data paradigm needs to be much more in line with the
  nmigen "Value" object (i.e. the nmigen API, such as
  using __iter__ instead of members() (or perhaps ports()),
  having an eq (instead of "assign") function etc.
  also note that Record is going to have a "connect" function
  added (see nmigen-migen Record compat library)

  in discussions with whitequark it was agreed that at some
  point a "flattening" system would be provided such that
  the nmigen IR "Visitor" system could handle arbitrary objects.

  therefore it becomes even more important that Data conform
  to nmigen's API, to avoid the "Principle of Least Astonishment"
  when presented with the Data paradigm:

  https://en.wikipedia.org/wiki/Principle_of_least_astonishment


EntryPort (and ExitPort)
---------

* o_ready and
* i_valid names were selected due to their vertical alignment.
  this increases code clarity.

* Signals default to width 1.  the addition of the width 1 is not needed.

* the name "EntryPort" begins with the letter E, and so does "ExitPort".
  this is problematic from a clarity perspective in later usage
  (see below)

* use of Data.assign for ready_in/out and valid_in/out, they should just
  be an eq.

* connect_from_entry is not clear what it does, by way of it being
  an EntryPort connecting *to* an EntryPort.  PrevControl._connect_in
  is clear (by using the prefix underscore as a standard python convention
  equivalent to "protected" in c++) that it should not be used outside
  of the API, and its docstring warns against such.

  with the name connect_from_entry being so similar to connect_from_exit
  it is very confusing, particularly as they are the exact same names
  in ExitPort

similar logic applies to ExitPort

CombSeg
-------

* comment is good.  "side-effect-free data path", very clear.

* self.i and self.o (see above about Entry/Exit Port) get very confusing.
  the naming "prev" and "next" (p and n for short) give clarity about
  what shall be connected to what, particularly in the context of
  ready/valid signalling going *both* directions.

* the use of the function create_data *on* an object - rather than being
  a function itself - forces a requirement on chaining of segments to
  provide an *intermediary* data object from which *both* the input
  object *and* the output object are forced to inherit.

  by simply having a function, the location of that function is irrelevant.
  it could be a globally-imported function from a module.  it could be
  a lambda function.  it could be a member class function.  it could be
  a static class function.

  this is what is meant by *semantic* relationships (LSP) as opposed to
  *syntactical* relationships.

* the use of an object (shape.create_data) *may* make the use of
  static classes difficult. needs evaluation and investigation

* the hard requirement for all derivatives to be modules is unnecessary
  (particularly in simple cases)


CombSegFn
---------

* Data.eq not Data.assign is better, although just "eq" is preferred
  (which can be used as "from import library import data" then "data.eq(...)")

* not sure what this class is for.  the large amount of data typing
  information is interfering with being able to understand it.

* once removed, the typing information should result in such a small
  class that it may turn out to be unnecessary.


CombChain
---------

* data typing is interfering with readability

* first_child can be merged into children in the constructor,
  making children_tuple redundant

* use of a tuple and the forced requirement that the tuple be an array
  of CombSegs is a design-flaw hard and unnecessary limitation.

  simply passing in *children and assigning them to __children will
  allow developers to pass in anything that is iterable.  generators,
  __iter__-objects, tuples, lists, sets, dictionaries (iterating a
  dictionary returns its values) and so on.

* in elaborate() the assignment of submodules is a good idea... however
  it assumes that the object *has* an elaborate function (i.e. is
  a nmigen module), which is a burdensome requirement in simple cases

* the functionality StageChain._specallocate is missing.  some use-cases,
  if the creation of an intermediary from a module is not done, the
  entire module is "flattened" with a warning issued.


Identity
--------

* not sure what this is for


Block
-----

* self.entry and self.exit are non-matching lengths, resulting in
  loss of vertical alignment when reading and debugging the code

* reducing these to self.e and self.e is clearly not going to work

* this is why "p" and "n" were chosen in ControlBase.

* self.p and self.n are clearly "previous" and "next" yet at the same
  time provide a base to put the "input" (i) and "output (o) into.

  data is referred to as self.p.i_data and self.n.o_data which make
  it really really obvious that input data comes from the *previous*
  stage and output data goes to the *next* stage.

* why does Block.wrap exist? the need for Block and BlockLike is not
  clear, and is probably down to the (unnecessary) type-restricting


CombBlock
---------

* why does CombBlock exist?

* why is connect_from_entry not being used in elaborate()?


Chain
-----

* all the same arguments applies to first_child and children as to
  CombChain. can be greatly simplified.

* likewise same arguments apply to elaborate()

* naming convention connect_from_entry / connect_from_exit is causing
  the very confusion predicted to occur.  what's being connected to
  what?

* the exposure of the inner details of Module is unnecessary.  an
  example of the equivalent code for the singlepipe.py API is:

    def elaborate(self, platform):
        m = Module()

        pipe1 = ExampleBufPipe()
        pipe2 = ExampleBufPipe()

        m.submodules.pipe1 = pipe1
        m.submodules.pipe2 = pipe2

        m.d.comb += self.connect([pipe1, pipe2])

  in this code it is really clear, the pipes are declared, they're added
  with names *chosen by the developer* as submodules, and then connected.

  by contrast, in Chain.elaborate(), the developer has no choice about
  the names of the modules (they are forcibly called child_0/1/2/3).
  (likewise for the names of CombChain segs)


SimpleReg
---------

* Signal(1, reset=0), both width 1 and reset=0 are the defaults and may
  be removed

* super().__init__(shape, shape) is not clear

*  shape = Shape.wrap(shape) is not clear

* shape.create_data() again, see explanation about forcing the use
  of an object to become part of the API rather than allowing freedom
  and flexibility by way of a function

* self.data_out does not vertically line up with self.valid_out.

  - self.o_data lines up with
  - self.o_valid

  at least at the prefix.  code that was sequentially as follows
  would also line up, improving readability:

  self.o_data
  self.i_data
  self.o_valid
  self.i_ready

  which would make a user consider tidying them up as follows:

  self.o_valid
  self.o_data
  self.i_data
  self.i_ready


Queue
-----

* like this class a lot, prefer that it be derived from FIFOInterface,
  to make it clear that it conforms to the nmigen FIFO API.

* Queue entries can actually be zero, as long as fwft (first-word
  fall-through) is enabled.

* using the name "fwft" as opposed to "allow_comb" makes it clear
  that it has the same purpose and effect as fwft in the nmigen FIFO API.

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