-
Notifications
You must be signed in to change notification settings - Fork 263
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
New feature proposal: a codified dynamic type system #521
Comments
Looks interesting! The best way forward would be to take small value adding steps and see where this goes. For the first step I would suggest that you create constant name_of_type : type_t := new_type_t("name_of_type"); To start with there is really no need for anything but a name property. Should consider making the ID a string to avoid some run-time encoding. At this point there is no difference between The second step would be to define an
I'm not sure but @kraigher probably knows. It may have to do with memory performance/utilization. One large string vs many small fragmented strings.
If
With the range being part of the code today it's probably easier to keep them together. If so, why not keep all together?
The true/false values comes from a time when we also supported humanly readable codecs for debugging. That could be changed. The
If the element is in itself an encoded string this would be easy. Finally, you could define
Yes, there should be many possibilities. All standard data structures should have a generic variant.
I'll have to dig more into what @umarcor have done to understand the implications. I let him respond to this |
My original thought was that there would be a problem defining ownership. If
Part of my motivation here is to facilitate I've actually already completed and tested a refactoring of One of my main motivations here was to bring symmetry to the structure of the two packages and to keep the algorithms for encoding and decoding a data type adjacent to each other in the same package. It was a real pain when you had to flip back and forth between packages to see how something was encoded and decoded. I also took the opportunity to clean up some of the algorithms. For example, I greatly simplified the algorithms for
I don't want to encourage it, but I think all tools are valuable if used appropriately. I'm sure there are some valid use cases. However, it would be reasonable to issue strong warnings about it in the documentation. I'll put together a useful addtion and make a pull request. |
Also, you didn't answer one of my Gitter questions. Some of my changes to |
This is coherent with some comments in the sources of string_ptr_pk and integer_vector_pkg, where the size of the allocated buffers is increased 'speculatively' to avoid too many allocations.
I must admit I'm a bit overwhelmed with this issue. I have not dig into queue and codec sources yet, so I'm blind. Anyway, I'll do my best: Currently,
This is all built on top of a new type, When a new vector is requested, a new element is added to Then, for each get, set, length, resize, etc. a 'case when' is used to do up to three different actions (depending on the type).
This is implemented in #507 for Then comes #470, which tries to extend the mechanism to memory_t in verification_components. As a matter of fact, this is the feature that motivated the series of PRs (see #462). This is where the fun begins!
Please see this comment by @kraigher, where he explains some very basic details of the memory model which I didn't know at the moment. Then, from my last comment there:
Hence, I believe that these same possibilities might apply for any other type that is built on top of either string_ptr or integer_vector_ptr. Each time you allocate one of them, you can decide to do it internally, as an external access or to be used through functions. However, I don't know when is queue_t used, and I think I'm mixing it with this comment. Is it possible that when @kraigher said that verification components interact with the memory model, he was referring to memory-mapped verification components? I.e. stream verification components use queue only and do not interact with memory at all. If so, I would say that queue 'must' support external models to be coherent with memory; so that verification components of any kind can be externally loaded. Precisely, I use AXI Stream VCs quite a lot. When data is allocated externally, I have a simple for loop in the testbench that pushes it to an AXI Stream Master, and another loop that pulls results from an AXI Stream Slave. This is not a problem due to the streaming nature of the flow. I.e, I can write/read a value per clock cycle, because I know that they are sequential. With a (RAM) memory, I need to copy everything before the actual testbench starts. Nonetheless, after this discussion, I believe that copying data from an external buffer to an AXI Stream verification component is not required. It should be possible to tell the verification component that data is already allocated and how many data elements are available. Regarding the 'codified dynamic type system', I'm getting lost with the 'codified' term. For me, such a feature should allow the user to define a record type in VHDL and an equivalent struct in C. Then, allocate an array of size number_of_elements*bytes_per_struct bytes, and have C access it through casting and GHDL use it as the 'memory' of a queue. I feel that you have tried to answer and explain this explicitly in the first comment, but I am not being able to understand how do you 'cast' an Last, but not least, the external models for As a side but related note, I think that currently vectors are given an index when they are allocated and the index is never reused, even if the vector is deallocated. As a result, the user is 'responsible' of allocating a sensible number of pointers and reusing the identifiers for an optimal solution. I believe it could be an interesting enhancement to keep track of the deallocated indexes without adding other three vectors to EDIT
My current proposal is to only implement string_ptr (1 byte) and integer_vector_ptr (4 bytes), because that's already quite a lot of code duplication (4 files are almost identical). However, if you find it useful, it can be implemented for a type of 8 bytes too: https://ghdl.readthedocs.io/en/latest/using/Foreign.html#restrictions-on-foreign-declarations |
Let's see how it turns out. In this case there are no VHDL-93 issues
Great
Yes, we've supported it long enough. I think we should make a commit where all deprecated stuff in |
@LarsAsplund, I remembered the issue that motivated my distinction between I think it would be elegant and unifying to use some version of Suppose a user wants to push an integer and that There are a few VHDL-93-compatible alternatives here, but I don't think either is good.
With VHDL-2008, this is a non-issue. ** @umarcor I'm not totally sure that If we were to change the implementation of |
I just took a deeper look at Changing the implementation to use a circular buffer would cut down on the number of times you need to resize. There would be a little more bookkeeping internally, but I think that would be minor relative to the benefits of a circular buffer. It would probably also make sense to stick to power-of-two buffer sizes. I might try to re-implement |
I had another realization about the current implementation of resize(queue.data, 2 * length(queue) + 1, drop => head); Suppose a 24-bit To push that string to the queue, the current implementation has to resize itself five times, from 0 to 1, 3, 7, 15, and finally 31 characters. It gets better once the string is of a reasonable length, but it has to reallocate and copy itself several times to bootstrap to a reasonable length. Just initializing the string to some reasonable length could increase performance. I'd like to get @kraigher's thoughts on this when he returns. |
Sorry for the spam, but I want to jot these ideas down while I have them. We can extend the reformulation of This change would also require that we modify the |
@LarsAsplund, I now have a working implementation of |
@bradleyharden, did you do those changes on top of #507 or independent to them? |
No, I did not consider it. It wouldn't be hard to merge them though. All I did is add an integer_vector to use as a stack for old pointer reference indices. The changes are just meant to get me to the point where I can test the performance difference between implementations of queue_t. We'll see what happens after that. |
@LarsAsplund and @kraigher, I have some preliminary performance tests of my new implementation of My version of
So far, it looks like my implementation has better performance overall. I've run two types of tests below. Here is an example of the "Push then pop" test for i in 0 to 2**16 - 1 loop
push(queue, 0);
end loop;
for i in 0 to 2**16 - 1 loop
int := pop(queue);
end loop; And here is an example of the "Push/pop" test for i in 0 to 2**16 - 1 loop
push(queue, 0);
int := pop(queue);
end loop; Here are the results for the
And here are the results for my implementation
So far, my implementation is better in every test. The advantage of a circular buffer is especially evident in the "Push/pop" tests. My implementation never has to resize the buffer, but the current implementation has to resize it regularly. Are there any other performance tests you would like me to try? With your permission, I would like to clean up my implementation and push it. The new |
@bradleyharden, that sounds really good. I'm looking forward to read the PR! |
Great ideas, but it seems like nothing has happened for a few years? |
No, this has been sleeping for quite some time but I think it will be revisited as we continue with our Python integration in #986. |
I mentioned this on Gitter, but I think it might be best to create an issue to consolidate the discussion and make sure comments don't get lost.
I would like to develop a generic dynamic typing system. I'm drawing inspiration from the
queue_element_type_t
and trying to generalize and expose that as a codified interface. My goal is to provide a generic type mark that contains everything you need to know to encode and decode a value of that type. What I'm aiming for is something like thisFor scalar types, size represents the number of bytes to encode a value. For array types, size represents the number of bytes to encode a single element of the array. Variable types have undetermined size. We can provide a function to determine the encoded length of any type
However, I will probably end up using a numerator and a denominator instead of a real, since the size is always a rational number. That way we can use integer division to calculate the size.
My initial thought was that you could declare all types as constants of this record type. However, to use it as a type mark would require that you always encode the entire record with the encoded data. Instead, it would be easier to implement it as
Then, we can use the type ID as an index into an array of
string_ptr
for getting the type name, an array oftype_class_t
for getting the type class, and an array ofreal
for getting the size. This is the same strategy asmsg_type_t
.Next, we can define an element
I'm not sure whether to keep the range as a
range_t
, i.e.bit_vector
, or to store it as an encoded range. It probably makes more sense to store it asstring(1 to 9)
. The simulator's internal representation ofrange_t
will need to be at least 9 bytes anyway, and then the size doesn't vary based on the range.You could convert between
element_t
and all other types with the functionsto_element
andfrom_element
, which would essentially be light wrappers aroundencode
anddecode
. The aliases forfrom_element
could also beto_std_logic_vector
, etc. to mimic a real type conversion.With this record, we can reformulate
queue_pkg
to push and pop elements. The string representation inside the queue wouldn't change, but every push and pop would be forced throughpush_element
andpop_element
.In fact, that brings up a separate point. Why is
queue_t
implemented with a single, variable lengthstring
? Why not implement it as a variable lengthinteger_vector
and treat each integer as astring_ptr
to an element? Then you never have to usepush_variable_string
and you also know the how many elements are in the queue at any moment. To me, that seems like a much better way to implement it. Is there some reason the current implementation was chosen instead?Anyway,
element_t
is great for pushing things around internally, using unconstrained function inputs and return values, but it isn't easy for users at the top level. There, I think it makes more sense to give themI'm not sure whether to use separate string pointers for the range and code here, or to combine them into a single
string_ptr
. Either way, this record is of fixed size and can be easily manipulated at the top level.With
element_t
andelement_ptr_t
you could perform type conversions of the underlying coded data. To facilitate that, I would like to change the encoding of all base types to use the standard binary representation. For example, instead of using'T'
and'F'
for aboolean
, usecharacter'val(0)
andcharacter'val(1)
That way, you could easily convert betweenboolean
and aunit8
type. Or you could convert areal
type into an array of 4uint16
types. To accomplish this withreal
, we could use theto_real
function fromfloat_pkg
, rather than the custom implementation right now.Next, you can define
array_t
as an array ofelement_t
, wherearray_t
stores each element internally as a pointer to an encoded string. In effect,element_t
acts as the container that @kraigher mentioned one time on Gitter when discussing arrays of arbitrary type.Finally, you could define
Using this, users could dynamically create a new
type_t
and append elements to thedynamic_t
to define their own containers for custom records.Also,
dict_t
could be modified to acceptelement_t
anddynamic_t
as keys and values. You could also create alist_t
based onarray_t
with a focus on list operations rather than a general array type, likeinteger_array_t
. I think there are many different directions you could take this.Separately, I also have an idea for a
bit_array_t
. Its purpose would be to store elements of arbitrary bit width. Internally, the data would be stored as one largestring
. Eachbit_array
would store arange_t
that indicates the size of each element. Theget
andset
functions would pull out a sub string and decode it to set/get the desired bits. You could even change therange_t
dynamically to change your view of thebit_array
. This is similar toread_word
andwrite_word
inmemory_t
, but not restricted to 8-bit boundaries. @umarcor might be interested in this as the basis for co-simulation. I think it would be the most flexible option.The text was updated successfully, but these errors were encountered: