Bug 351 - create a "block" (mass) regfile port (read and write) onto an array-based regfile
Summary: create a "block" (mass) regfile port (read and write) onto an array-based reg...
Status: RESOLVED FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Mac OS
: --- enhancement
Assignee: Luke Kenneth Casson Leighton
URL:
Depends on:
Blocks: 383 345
  Show dependency treegraph
 
Reported: 2020-05-25 15:07 BST by Luke Kenneth Casson Leighton
Modified: 2020-12-02 20:37 GMT (History)
3 users (show)

See Also:
NLnet milestone: NLNet.2019.10.Wishbone
total budget (EUR) for completion of task and all subtasks: 200
budget (EUR) for this task, excluding subtasks' budget: 200
parent task for budget allocation: 383
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
cole={amount=100,submitted=2020-09-20,paid=2020-10-01} # TODO(lkcl): correct date not known lkcl={amount=100,paid=2020-10-01}


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2020-05-25 15:07:14 BST
see soc.regfile.RegFileArray

for the Condition Regfile and for the XER Regfile, a "mass-hit" read/write
port is required that looks like a standard read_port (or write_port) however
permits writing to the *entire* regfile, all at once.

this can be done as a virtual port, as follows:

* create 8x read (or write) ports
* join all 8 data together as if they were a single thing
* join all 8 "enable" lines together as if they were a single thing

it will however be critical to ensure that the alternative "view" is not
used at the same time.
Comment 1 Luke Kenneth Casson Leighton 2020-05-26 00:55:07 BST
ok so first derive from RegFileArray.

use the two parameters as follows:

* bitwidth: the TOTAL number of bits
* n_regs: the number of subdivisions of those bits.

therefore RegfileArray n_regs = nregs

however RegfileArray regwidth is bitwidth DIVIDED by n_regs.

then, in the constructor, add n_regs read ports and n_regs write ports to a pair of lists.

you can see this being done in soc.regfile.regfiles here

https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/regfile/regfiles.py;h=d6f3a8dd79930d1286c72a42328363d9b9b8e9f3;hb=HEAD#l42

except, that should be a for loop


next, we want an EXTRA read port and an extra write port.

this should, for data, instead of being only bitwidth/nregs wide, should instead be the FULL bitwidth wide. 

however the enable lines should be nregs wide.

39         port = RecordObject([("ren", nregs),
  40                              ("data_o", bitwidth)],
  41                             name=name)

like that.


then, in the elaborate:

* if any of the "extra" port ren bits are set, the data is sent through the corresponding *lower* indexed ports.

you see how that works?
Comment 2 Luke Kenneth Casson Leighton 2020-05-26 02:18:36 BST
https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=HEAD

saw this.


i think... actually... the list  of ports in the constructor, have to be done manually, then those wired via a mux to the "real" ports

so a for loop adds RecordObjects

ren=1, data=bitwidth/nregs

then appends an *extra* on
ren=nregs, data=bitwidth


then in the constructor:

* test port[-1].ren == 0

* if zero then for all ports 0 to nregs, connect *internal* port to external port

* else put bitwidth/nregs bits of port[-1].data onto port[loopindex].data and also do same with enable bits.
Comment 3 Cole Poirier 2020-05-26 02:34:13 BST
(In reply to Luke Kenneth Casson Leighton from comment #1)

Unfortunately I had quite a lot of difficulty trying to follow your instructions. I was unable to determine what you were referring to in several instances, and grepping for some of the terms used turned up no results in the codebase. I went ahead and pushed my commit as soc/regfile/virtual_port.py, its my best attempt at interpreting your instructions, but its god ugly and I felt bad about committing such a weak skeleton, but remembered your earlier guidance to push so it's easier to get help. Hopefully with some clarifying questions below I'll be able to better learn to speak the language of the project :)

> ok so first derive from RegFileArray.

As in regfiles.py "class IntRegs(RegFileArray):"?

> use the two parameters as follows:

By 'the two parameters', do you mean inherited from RegFileArray self.width and self.depth? Otherwise do you mean take the below two parameters as new parameters in the __init__ of VirtualPort, like "def __init__(self, bitwidth, n_regs):'? Also do I have to "super().__init__(m, n)" before assigning python lists to self.w_ports and same for rd?

> * bitwidth: the TOTAL number of bits
> * n_regs: the number of subdivisions of those bits.
> 
> therefore RegfileArray n_regs = nregs

I'm very confused by this, is n_regs internal to RegFileArray while nregs is derived from n_regs for VirtualPort?

> 
> however RegfileArray regwidth is bitwidth DIVIDED by n_regs.

Where is regwidth used? I assume it's important but I am missing/have missed something.

> then, in the constructor, add n_regs read ports and n_regs write ports to a
> pair of lists.
> 
> you can see this being done in soc.regfile.regfiles here
> 
> https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/regfile/regfiles.py;
> h=d6f3a8dd79930d1286c72a42328363d9b9b8e9f3;hb=HEAD#l42
> 
> except, that should be a for loop
> 
> 
> next, we want an EXTRA read port and an extra write port.

Does this mean that I should keep them as separate variables outside of the wr_ports and rd_ports lists as I have done in my first commit for this?

> this should, for data, instead of being only bitwidth/nregs wide, should
> instead be the FULL bitwidth wide. 
> 
> however the enable lines should be nregs wide.
> 
> 39         port = RecordObject([("ren", nregs),
>   40                              ("data_o", bitwidth)],
>   41                             name=name)
> 
> like that.
> 
> 
> then, in the elaborate:
> 
> * if any of the "extra" port ren bits are set, the data is sent through the
> corresponding *lower* indexed ports.

I was unable to determine how to route the data through the corresponding lower indexed ports, I suspect it might have something to do with the below code from regfile.py, but I was unable to make heads or tails of it either way:
```
144  for (regs, p) in self._rdports:
145     #print (p)
146     m.d.comb += self._get_en_sig(regs, 'ren').eq(p.ren)
147     ror = treereduce(list(regs))
148     m.d.comb += p.data_o.eq(ror)
```

Some explicit guidance here in particular would be really helpful, as I think this might be the thing I have the least idea of how to go about implementing. I assume that my function should match each bit to its corresponding 'enabled' port but I'm only about 10% sure which variable or 'named parameter' I access this through.

I apologize that I just don't understand a good portion of your instructions, but I think that's because I'm learning how to understand and communicate in the esoteric language of hardware, as well as the terminology that is specific to our project. If it's something else, I'd sure like to know so I can work to remedy it.

> you see how that works?

Hehe I think, as you can see, I don't, but I hope to through working through this :)
Comment 4 Cole Poirier 2020-05-26 02:45:41 BST
(In reply to Luke Kenneth Casson Leighton from comment #2)
> https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=HEAD
> 
> saw this.

Thanks! I'm trying to get something down, so I don't freeze because there are multiple things I don't understand.

> 
> i think... actually... the list  of ports in the constructor, have to be
> done manually, then those wired via a mux to the "real" ports
> 
> so a for loop adds RecordObjects
> 
> ren=1, data=bitwidth/nregs
> 
> then appends an *extra* on
> ren=nregs, data=bitwidth

Pushed a commit with this change, not sure if it's exactly right yet.

> 
> then in the constructor:
> 
> * test port[-1].ren == 0
> 
> * if zero then for all ports 0 to nregs, connect *internal* port to external
> port
> 
> * else put bitwidth/nregs bits of port[-1].data onto port[loopindex].data
> and also do same with enable bits.

Can you help me undestand how I should set the interal and external ports up in the __init__() function? I'm lost here as to how these should be represented in code.
Comment 5 Luke Kenneth Casson Leighton 2020-05-26 03:03:56 BST
(In reply to Cole Poirier from comment #3)
> (In reply to Luke Kenneth Casson Leighton from comment #1)
> 
> Unfortunately I had quite a lot of difficulty trying to follow your
> instructions.

v. late and using phone. sorry.
Comment 6 Cole Poirier 2020-05-26 03:08:46 BST
(In reply to Luke Kenneth Casson Leighton from comment #5)
> (In reply to Cole Poirier from comment #3)
> > (In reply to Luke Kenneth Casson Leighton from comment #1)
> > 
> > Unfortunately I had quite a lot of difficulty trying to follow your
> > instructions.
> 
> v. late and using phone. sorry.

No problem, end of day here. I will be back online at 1600 utc (9am pst or daylight whatever, NA west coast time). If the time sensitivity of this is within that time period feel free to complete this in my absence, otherwise, see you tomorrow afternoon (for you).
Comment 7 Luke Kenneth Casson Leighton 2020-05-26 03:09:17 BST
(In reply to Cole Poirier from comment #4)
> (In reply to Luke Kenneth Casson Leighton from comment #2)
> > https://git.libre-soc.org/?p=soc.git;a=commitdiff;h=HEAD
> > 
> > saw this.
> 
> Thanks! I'm trying to get something down, so I don't freeze because there
> are multiple things I don't understand.
> 
> > 
> > i think... actually... the list  of ports in the constructor, have to be
> > done manually, then those wired via a mux to the "real" ports
> > 
> > so a for loop adds RecordObjects
> > 
> > ren=1, data=bitwidth/nregs
> > 
> > then appends an *extra* on
> > ren=nregs, data=bitwidth
> 
> Pushed a commit with this change, not sure if it's exactly right yet.

not seen it.

2nd commit needs to add a *SECOND* pair of lists.

to 2nd set, *manually* add RecordObjects.  do not use readport() or writeport()  function.  *MANUALLY* add multiple RecordObjects.

these are the "external" where the 1st set is "internal".

> > 
> > then in the constructor:

i meant "elaborate" here.
Comment 8 Jacob Lifshay 2020-05-26 05:07:09 BST
(In reply to Cole Poirier from comment #6)
> No problem, end of day here. I will be back online at 1600 utc (9am pst or
> daylight whatever, NA west coast time).

Pacific Daylight Time (PDT)?
Comment 9 Luke Kenneth Casson Leighton 2020-05-26 13:54:41 BST
ok i am more awake.  started on adding the virtual port aaaand... spangg, i
ran smack into a pre-existing very close approximation of the virtual port,
in the form of RegFileArray.

*face-palm*.

the way that RegFileArray works is: it creates an array of registers,
then when read_port (or write_port) is called, an *array* of individual
read/write ports - one for each individual register - is returned, *and*
a "global" access port is provided.

that global access port is a "broadcast bus" to every register, and its
width is the *exact* same width as the register width.

also, where each register has a 1-bit-wide ren/wen, the global port is
the *accumulation* of all those signals (using self._get_en_sig) in a
unary sequence.

thus, if you pass in say a global.ren="0b00001011", this will read out the OR
of registers numbered 0, 1 *and* 3 (!!) as in:

    result = reg[0] ORed with reg[1] ORed with reg[3]

this will be a regwidth-wide result

what we want, for the virtual port is:

    result = reg[0] CATted with reg[1] CATted with reg[2] CATed with reg[3]...

this will be a regwidth TIMES n_regs wide result

which is very different.
Comment 10 Luke Kenneth Casson Leighton 2020-05-26 17:01:01 BST
https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/regfile/virtual_port.py;hb=HEAD

i think i have it.  i had to modify RegFileArray to first get it to give me
only (an array of) read/write ports to each (small) register.

however... *this is not the full story*!

we need to prevent and prohibit RegFileArray.read_port() and
RegFileArray.write_port() from having a "bypass" on the registers

this is complicated.
Comment 11 Cole Poirier 2020-05-26 17:41:00 BST
(In reply to Luke Kenneth Casson Leighton from comment #10)
> https://git.libre-soc.org/?p=soc.git;a=blob;f=src/soc/regfile/virtual_port.
> py;hb=HEAD
> 
> i think i have it.  i had to modify RegFileArray to first get it to give me
> only (an array of) read/write ports to each (small) register.
> 
> however... *this is not the full story*!
> 
> we need to prevent and prohibit RegFileArray.read_port() and
> RegFileArray.write_port() from having a "bypass" on the registers
> 
> this is complicated.

I take it you've got this bug under control and I should move on to something else today? :)
Comment 12 Luke Kenneth Casson Leighton 2020-05-26 23:20:33 BST
urr ok i removed the mux on the virtual file port, it's now "direct" and only
the wide (full) path is possible.  now the next important thing is to have a
test which shows there is no conflict or data corruption when accessing
"small" ports vs the big one.
Comment 13 Luke Kenneth Casson Leighton 2020-05-27 00:17:56 BST
added a "demo" unit test that seems to show that it's functional.  i added
one extra read port and HOLY cow did the number of gates jump.  that's just
a 32-bit register.  luckily we only need one of them.
Comment 14 Jacob Lifshay 2020-10-02 00:04:28 BST
Did NLNet also pay luke today? because otherwise you should use `submitted` instead of `paid` if Luke submitted an RFP today, or just leave only `amount` if neither happened yet.

I also fixed typos.
Comment 15 Cole Poirier 2020-10-02 00:07:05 BST
(In reply to Jacob Lifshay from comment #14)
> Did NLNet also pay luke today? because otherwise you should use `submitted`
> instead of `paid` if Luke submitted an RFP today, or just leave only
> `amount` if neither happened yet.
> 
> I also fixed typos.

Thanks Jacob! I don't know the date Luke was paid, but it is listed as paid on his wiki page. What should I put in that case?
Comment 16 Jacob Lifshay 2020-10-02 00:11:10 BST
(In reply to Cole Poirier from comment #15)
> (In reply to Jacob Lifshay from comment #14)
> > Did NLNet also pay luke today? because otherwise you should use `submitted`
> > instead of `paid` if Luke submitted an RFP today, or just leave only
> > `amount` if neither happened yet.
> > 
> > I also fixed typos.
> 
> Thanks Jacob! I don't know the date Luke was paid, but it is listed as paid
> on his wiki page. What should I put in that case?

Add a comment something like what I just added.

Also, Luke, please try to keep bugzilla updated since budget-sync is using it as the canonical source of truth.
Comment 17 Cole Poirier 2020-10-02 00:25:57 BST
(In reply to Jacob Lifshay from comment #16)
> (In reply to Cole Poirier from comment #15)
> > (In reply to Jacob Lifshay from comment #14)
> > > Did NLNet also pay luke today? because otherwise you should use `submitted`
> > > instead of `paid` if Luke submitted an RFP today, or just leave only
> > > `amount` if neither happened yet.
> > > 
> > > I also fixed typos.
> > 
> > Thanks Jacob! I don't know the date Luke was paid, but it is listed as paid
> > on his wiki page. What should I put in that case?
> 
> Add a comment something like what I just added.
> 
> Also, Luke, please try to keep bugzilla updated since budget-sync is using
> it as the canonical source of truth.

Ah, very straightforward, will do. Thanks Jacob.