[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 #8 from Jacob Lifshay <programmerjake at gmail.com> ---
(In reply to Luke Kenneth Casson Leighton from comment #4)
> review of
> https://salsa.debian.org/Kazan-team/simple-barrel-processor/blob/master/src/PipelineBuildingBlockAPIProposal.py
>
> can the typing be moved to a .pyi file and mypi deployed *if
> the user wants to use it*?
yes.

> Data, Shape, SignalShape, RecordShape, Empty, EmptyShape
> ----------.....
> 
> * addition of name extension (prefix) is a good idea.  however
>   as a hard requirement (a hard-required parameter) this introduces
>   a burden on developers.  if the same prefixing can be achieved in
>   the same way as nmigen, that would be even better.
yeah, we can add that.

> * 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.
We can override __subclasshook__ like in collections.abc.Hashable:
https://github.com/python/cpython/blob/1b5f9c9653f348b0aa8b7ca39f8a9361150f7dfc/Lib/_collections_abc.py#L84

This makes any class that implements the 2 abstract functions to be a subclass
of Data automatically (except that Data's methods won't be visible unless
actually deriving from Data).
> 
> * 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)
this will require every new class to actually derive from Data unless they want
to implement the zillion functions themselves.

Note that, last I checked, nmigen doesn't support using classes derived from
Signal or Record since nmigen uses `type(a) is type(Signal)` instead of
`isinstance(a, Signal)` in a lot of it's code.

> EntryPort (and ExitPort)
> ---------
> 
> * o_ready and
> * i_valid names were selected due to their vertical alignment.
>   this increases code clarity.
we can use _inp and _out or _i and _o. I think we should have the name first
(direction second) because the name is the most important part

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

> * the name "EntryPort" begins with the letter E, and so does "ExitPort".
>   this is problematic from a clarity perspective in later usage
>   (see below)
I wanted to pick a name pair other than input/output to avoid confusion with
the signal directions.
What about ingress/egress, from/to, upstream/downstream, or next/previous? Or
do you think input/output will be fine?
> 
> * use of Data.assign for ready_in/out and valid_in/out, they should just
>   be an eq.
They can't be a member function because Record.eq doesn't work (the simulator
doesn't handle it), though I guess we could reassign Record.eq to a custom
function that does work. 

> * 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
They need better naming. connect_from_* connects such that data is transferred
from `rhs` to `self`.

> 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.
CombSeg has no ready/valid signalling, it's just a data-path.

> * 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.
No, it doesn't actually. all it requires is that the output shape of the
previous CombSeg == the input shape of the current CombSeg, which is a
structural comparison, not identity.

Note that Shape.wrap is used which converts whatever shape you pass in to an
instance of Shape. only the Shape instance has to support create_data, whatever
value you pass in does not.

>   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)
It doesn't add any extra logic gates to the final design and it helps when
finding the generated code for the output to be split up.
I don't want to have to debug a module made from 20 stages, because I won't be
able to easily locate anything.

> 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(...)")
I'm using Data as a place to stash lots of Data-related static functions. We
can switch to a module if you like.

> * not sure what this class is for.  the large amount of data typing
>   information is interfering with being able to understand it.
this is for wrapping functions into CombSeg instances.

> CombChain
> ---------
> 
> * data typing is interfering with readability
> 
> * first_child can be merged into children in the constructor,
>   making children_tuple redundant
ok. I had it to enforce at least 1 child.

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

>   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.
you can do that already by calling CombChain(*iterable)

> * 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
in simple usecases just use CombSegFn(lambda ...)

> * 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.
not sure what that's supposed to do.

> Identity
> --------
> 
> * not sure what this is for
when you need a canonical pass-through stage. probably not particularly useful.
> 
> 
> Block
> -----
> 
> * why does Block.wrap exist? the need for Block and BlockLike is not
>   clear, and is probably down to the (unnecessary) type-restricting
Block.wrap is a function that takes an instance of Block or CombSeg and returns
an instance of Block. It is needed because CombSeg does not have the
ready/valid signalling that Block instances do.
> 
> 
> CombBlock
> ---------
> 
> * why does CombBlock exist?
the class used by Block.wrap to wrap a CombSeg
> 
> * why is connect_from_entry not being used in elaborate()?
connect_from_entry only works on instances of EntryPort. CombSeg doesn't
contain any instances of EntryPort because you had suggested that we need a
class that doesn't have accessible ready/valid signalling, hence why I had
added CombSeg at all and not gone with my original plan of having only Block.

> 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?
see above on renaming connect_from_*.
> 
> * 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)
Chain is supposed to be like a tuple or list. We can add an API for named
submodules either by modifying Chain or by adding a named tuple variant.

> SimpleReg
> ---------
> 
> * Signal(1, reset=0), both width 1 and reset=0 are the defaults and may
>   be removed
I'd like reset=0 to be left as it makes it explicit that the reset value
actually matters whereas most other signals the reset value doesn't really
matter.
so Signal(reset=0)

> * super().__init__(shape, shape) is not clear
both entry and exit data shapes are shape.

> *  shape = Shape.wrap(shape) is not clear
convert from arbitrary shape to Shape instance

> * 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
see above

> * 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
I have no opinion about the assignment order, reorder as you please.

> 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.
It doesn't necessarily conform to the nmigen FIFO API.

I want it to have the same interface as Block so it can be used in Block-based
pipelines without friction.

> * Queue entries can actually be zero, as long as fwft (first-word
>   fall-through) is enabled.
that would actually make it identical to Identity, so I think that won't be
useful.

> * 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.
allow_comb doesn't have the same purpose or effect as fwft. It specifically
allows combinational forwarding, but also enables more than that.
allow_comb=False specifically requires Queue to be a Moore FSM (outputs are
produced only from flip-flop outputs) instead of a Mealy FSM (outputs are
produced from a combinatorial combination of flip-flop outputs and input
signals).

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