[libre-riscv-dev] [Bug 64] data handling / io control / data routing API needed
bugzilla-daemon at libre-riscv.org
bugzilla-daemon at libre-riscv.org
Fri Apr 26 02:26:36 BST 2019
--- Comment #8 from Jacob Lifshay <programmerjake at gmail.com> ---
(In reply to Luke Kenneth Casson Leighton from comment #4)
> review of
> 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. 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
We can override __subclasshook__ like in collections.abc.Hashable:
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.
> * 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`.
> * 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.
> * Data.eq not Data.assign is better, although just "eq" is preferred
> (which can be used as "from import library import data" then
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.
> * 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.
> * not sure what this is for
when you need a canonical pass-through stage. probably not particularly useful.
> * 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.
> * 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.
> * 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
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.
> * 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
> * 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
> * 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:
> which would make a user consider tidying them up as follows:
I have no opinion about the assignment order, reorder as you please.
> * 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
> * 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
You are receiving this mail because:
You are on the CC list for the bug.
More information about the libre-riscv-dev