[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 #45 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
(In reply to Jacob Lifshay from comment #39)

> >  shape() is used to find out, from the *recursively-iterable*
> >  thing-of-Value-derivatives, what size of Signal() would be needed to store
> > it
> >  (as a sequence of bits)
> I think we should call it bitlen or something like that, to me shape implies
> returning the type of the Data rather than the number of bits inside it.

 yes.  agreed.  however...

> In C++ it would be sizeof vs. decltype.

 ...that needs to be a recommendation made to the nmigen developers, to change
 the nmigen API from "shape" to "bitlen".

 the nmoperator.shape function simply follows the nmigen convention: not
 doing so is... well... inadviseable: it will result in "surprises":
 https://en.wikipedia.org/wiki/Least_surprise

 so... i agree that yes, shape is a bad name... unfortunately, the name's
 already been chosen, and it would be required to be documented that the
 functionality is identical despite the name being different.

 the better choice, i feel, is to stick with the [poor] name-choice, "shape()"


> >  cat() is used to flatten() and *de*-flatten() the incoming and outgoing
> >  hierarchy-of-Value-derivatives so that ONLY ONE Queue object is needed.
> I think we should do that inside the Queue implementation (or better yet,
> inside Memory, which is what the original Chisel code does).

 mmmm this is an idea which, initially, i like, however it has knock-on
 consequences.

 the first is: Memory2 (or whatever it is called) now has a critical
 dependence on the nmoperator API, which would be the first "base"
 utility class outside of the Stage/Pipe API to do so.

 i'm... reluctant to set that precedent, particularly given that the API
 would change to no longer specify a width, it would need to specify
 the "spec".  i.e. it would need to be passed an "iospecfn".

 that's a really drastic change in the API, going beyond my comfort level
 for a long-term API


 second: the use of the flattening actually results in some truly
 dreadful graphviz representations, as the data gets jammed into a
 box of bits, then extracted on the other side in a truly awful messy
 way.


 third: the same logic that applies to Memory also applies to Queue.
 Queue's API would need to be changed, and i'm really not comfortable
 with that.

 right now, it's pretty damn obvious.  read the chisel3 code, read
 the comment about the name-changes, set the two pieces of code
 side-by-side and it's damn obvious (if you can read chisel3 that is)
 that the two are directly equivalent.

 that equivalence is DESTROYED by embedding a hard critical dependence
 on the iospecfn (data format) API.


 ... or...

 very simple...

 use what i called FIFOControl... because FIFOControl *is* Queue [with
 the flattening and processing added].

 in an earlier iteration of FIFOControl, this direct one-for-one equivalence
 was much more obvious, because i was able to make the input port DIRECTLY
 equal to a PrevControl, and the output port DIRECTLY equal to a NextControl.

 unfortunately due to the need to process the in/out data, that equivalence
 was broken, and i had to extract the contents of NextControl._connect_in
 and use those contents *in* FIFOControl.

 *grumble*.

 so basically:

 * if you don't want the nmoperator API to be involved, don't want
   the Shape API involved, don't want the pipeline API involved...

   ... use Queue

 * if you *do* want the data to be flattened...

   ... use FIFOControl.  they are EXACTLY and PRECISELY equivalent.
   ABSOLUTELY NO CHANGE IN THE ARGUMENTS OR THE LOGIC HAS BEEN MADE.  

 if this is not understood, see the comments in queue.py:

            din  = enq_data, writable  = enq_ready, we = enq_valid
            dout = deq_data, re = deq_ready, readable = deq_valid


 Memory has problems of its own (can't do length-1 arrays), which
 makes a Memory2 worthwhile considering doing (for different reasons)

 SyncFIFOBuffered should not exist, basically.


> We need at least one more function, create a new Data that shares the type
> of an existing one. I did that using Shape.get(data).create_data(), similar
> to python type(value)(). This is used to create Data instances for internal
> registers and what-not.

 you're thinking of Value.like.  Signal.like.  etc.

 yes, and the nice thing is: whilst Record.like does not exist (and it should),
 nmoperator.like() can substitute for that lack by auto-detecting if it is
 ever passed a Record.

 the only thing is: we'll need to actually write a recursive-Record-walker
 "like" function (as a non-member function).

 basically Record.like is really really simple: grab the fields member,
 and um create a new Record instance using that Record's fields.

 that's it.

 the only thing we have to watch out for is: if we want to support something
 that *derives* from Record, it should have its own "like" function so
 that we can test for it (if hasattr(x, "like")) and call it.

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