Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider arrays in Records #222

Closed
RobertBaruch opened this issue Sep 21, 2019 · 10 comments
Closed

Consider arrays in Records #222

RobertBaruch opened this issue Sep 21, 2019 · 10 comments
Labels

Comments

@RobertBaruch
Copy link

Instead of having to do this for 4 elements of 8-bit values:

class BagOfSignals(Layout):
  def __init__(self):
    super().__init__([
      ("data0", 8, DIR_FANOUT),
      ("data1", 8, DIR_FANOUT),
      ("data2", 8, DIR_FANOUT),
      ("data3", 8, DIR_FANOUT),
    ])

Maybe:

class BagOfSignals(Layout):
  def __init__(self):
    super().__init__([
      ("data", 8, 4, DIR_FANOUT),
    ])

or ("data", 4, 8, DIR_FANOUT) depending on how you would like your inner monologue to go: "data is 4 elements of 8 bits each, fanning out" probably makes more sense than "data is 8 bits for each of 4 elements, fanning out".

All elements in the array are assumed to have the same direction. If they don't, then you don't want an array, or at least you want two separate arrays.

Access should be like record.data[0], record.data[1], etc. I'm not sure what the names of the signals end up as. I've seen that Array ends up with $n as a subscript, so that would be fine as well.

@whitequark
Copy link
Contributor

I don't think this syntax is viable. It is bad enough to have a tuple in a Record as it is, I certainly do not think that extending it to have two unlabeled integers is a good idea.

@whitequark
Copy link
Contributor

What are the use cases for this feature? Submitting an MCVE for a bug is an excellent practice that I appreciate, but submitting an "MCVE" for a feature is not so good because I don't have the context that you presumably do, and so it is not possible for me to see if the use case truly warrants the increase in complexity.

@jordens
Copy link
Member

jordens commented Sep 22, 2019

I use homogeneous ndim arrays of signals and would like to see operations such as transpose or reshape. But imo arrays don't need to appear as a special case within a record. All the operations that I'm interested would probably be covered by (1) the transpose, flatten/ravel and reshape family of methods and (2) making lists of signals synonymous to their Cat() in RHS expressions.
This is with a perspective back on a long timeline of array implementations and heavy use of them in Python.

@RobertBaruch
Copy link
Author

In my use case, I'm building a Z80 core with formal verification. To do that, I need to record what an instruction does as it executes, and how many cycles it spends doing various tasks. See the Records I've set up, especially OperandLayout, AccessLayout, and CycleLayout.

So an instruction might do 2 reads of memory and 2 writes of memory, or maybe 4 reads of memory. Each of those memory accesses has an address and data. So such a Record would need to store the number of operations, and the data associated with each operation.

Each read or write takes a variable number of cycles, since the instruction may need to do some additional work. So we need to store the cycle type, and the number of sub-cycles spent in that cycle, for up to six cycles.

Why a Record? Because I need to (a) pass the data around several modules, and (b) compare the data with the expected data. This is most convenient using Records because then I can connect them easily.

@whitequark
Copy link
Contributor

All the operations that I'm interested would probably be covered by (1) the transpose, flatten/ravel and reshape family of methods and (2) making lists of signals synonymous to their Cat() in RHS expressions.

To me it sounds like you want something like a generalization of a matrix instead. (I'm not sure what the term for this entity is.) That's doesn't even need to be in core nMigen, as you can make anything Record-like (in the sense that it is valid RHS and LHS) using the UserValue that I've introduced recently. I think you can prototype it yourself easily.

@whitequark
Copy link
Contributor

whitequark commented Sep 22, 2019

@RobertBaruch I think this is is, in itself, a reasonable request. However, the existing Record layout syntax was never designed with this sort of complex use case in mind, nor are the Record internals easily extensible to handle something like an array with connect.

As such, I don't think there is a good way to add this functionality to Record. There will, in fact, likely not be any such way until Record is completely redesigned, either under the same name (and so accepting the tuples for compatibility), or under something else entirely, maybe a Struct.

I would suggest that as a workaround with better quality of life, you use Python facilities, something like:

@property
def data(self):
    return list(self.data0, self.data1, self.data2)

or perhaps an Array if that's what you need, as I see you are using Switch statements to assign one of the fields in your code.

In fact, you could also go ahead and use UserValue to implement your completely custom Record-alike that works exactly the way you want.

@jordens
Copy link
Member

jordens commented Sep 22, 2019

It's not a generalization of matrix to me (certainly not in the mathematical sense which is the one that numpy uses for a some 2d arrays). Transpose happens when re-framing data words or when casting parallel data in serdes applications. Reshape is what you do on wide memory or bus data when looking at a byte granularity write ena le. Those ops to me are genuinely associated with multi-bit signals themselves. And they would also be how I would tackle the use case described at the top.

@whitequark
Copy link
Contributor

Transpose happens when re-framing data words or when casting parallel data in serdes applications.

I don't understand what transpose does, then.

Reshape is what you do on wide memory or bus data when looking at a byte granularity write ena le.

Do you mean something like data.word_select(8, byteno) as currently implemented?

@jordens
Copy link
Member

jordens commented Sep 22, 2019

signal.transpose(m) would be pretty much Cat(signal[i::m] for i in range(len(signal)//m) with checking that sizes match.
signal.reshape(n, m) would be [signal.word_select(m, i) for i in range(n)] or even [signal[i*m:(i + 1)*m] for i in range(n)] (both with size match checks) with the natural extension to higher dimensions than 2.
Admittedly it's merely shorthand in most cases. But it does help readability a lot.
None of those ops are dynamic.

@whitequark
Copy link
Contributor

Oh I see. That seems pretty orthogonal to me here, since if you're OK with reshape you could also define an accessor like I suggested earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants