New queue implementation

[bradleyharden]

This is a first step toward #521. Introducing a new implementation of queue_t will allow me to create a new, generalized element_t or item_t that can represent any VHDL value. Performance tests indicate that this implementation is faster than the current implementation.

I'm creating a draft PR for now, so that you can start to review the changes. I tried to group the changes into 3 reasonable commits. I still need to update com to use support the new queue_pkg before this will be ready to pull.

[bradleyharden]

I've updated com and removed the old, deprecated code. I also took the opportunity to remove the pool packages, since they aren't needed anymore with my addition to integer_vector_ptr_pkg and string_ptr_pkg.

The acceptance tests ran fine on my machine. I'm hoping for no surprises. This should be pretty close to complete.

[bradleyharden]

I just want to be clear. There are some backwards incompatible changes here:

• The drop parameter is removed from the string_ptr resize function
• The drop parameter is changed to rotate in the integer_vector_ptr resize function
• All pool packages are removed

Is this enough to cause a major version increment? If so, perhaps I should continue working on #521 and we can commit everything at once?

[LarsAsplund]

@bradleyharden I haven't looked at the commit yet but when it comes to breaking backward compatibility we prefer keeping the old APIs, if not too inconvenient, as a deprecated approach for a while before removing them. I'm not sure how inconvenient it would be in this case.

[bradleyharden]

Keeping drop in string_ptr resize wouldn't be difficult. Keeping drop in integer_vector_ptr resize would be awkward, but not impossible. I removed these because I suspect that no one actually uses them. The only use of drop in the entire code base was in queue_pkg. However, I suppose someone out there could be using drop, so we can keep it in.

I can keep the pool packages as well, but I'll also keep my changes that remove their use throughout VUnit. Does that work? VUnit will no longer use them, but they will remain available for anyone who does.

[umarcor]

* The `drop` parameter is removed from the `string_ptr` `resize` function
* The `drop` parameter is changed to `rotate` in the `integer_vector_ptr` `resize` function

This caught my attention while revewing the changes. I'd expect drop to be removed from both types, or it to be changed to rotate in both of them. What's the reason to keep it in integer_vector but not in string?

Is this enough to cause a major version increment? If so, perhaps I should continue working on #521 and we can commit everything at once?

IMHO, the new major version should include:

• Modifications to queue and to stream. Update stream pkg #513, New feature proposal: a codified dynamic type system #521, New queue implementation #522
• Splitting VCs. (Re)move AXI and UART verification components to separate repos #511, Add compliance tests and updated VCs accordingly #520, Added verification components for SPI and AVMM #425
• External modes for string and integer_vector. [2/3] Add external modes to string_ptr and integer_vector_ptr (through VHPIDIRECT) #476, [1/3] Add external models to string_ptr; add byte_vector* as an alias #507, [PoC] Interfacing GHDL to other languages: declaration of foreign C functions #465, [3/3] Support up to N external memory models #470

I don't think we need to worry about pushing it too soon.

[umarcor]

@bradleyharden, I didn't see your last comment.

Keeping drop in string_ptr resize wouldn't be difficult. Keeping drop in integer_vector_ptr resize would be awkward, but not impossible. I removed these because I suspect that no one actually uses them. The only use of drop in the entire code base was in queue_pkg. However, I suppose someone out there could be using drop, so we can keep it in.

If keeping drop in string is not difficult, I think the way would be to do it and to rename rotate in integer_vector to drop. This would preserve backward compatibility, wouldn't it?

In integer_vector, is rotate functionally different from drop?

I can keep the pool packages as well, but I'll also keep my changes that remove their use throughout VUnit. Does that work? VUnit will no longer use them, but they will remain available for anyone who does.

Actually, I believe this is how deprecation should be managed. This would allow to merge this PR without introducing incompatibilities, wouldn't it?

[bradleyharden]

@umarcor, I suspect that drop was only ever added to integer_vector_ptr to mimic string_ptr, but string_ptr only has drop to support queue_t. I can't see many use cases for it.

rotate is different than drop. It greatly simplifies resizing a circular buffer. I could explain more, but the code would be more precise.

In the new version, rotate is only added to support queue_t. I thought it would make sense to only add extra functionality where there is actually a use case, so I left it off of string_ptr. Although, I suppose someone could implement a circular buffer of bytes and would need a rotate parameter for string_ptr.

I think the solution will be to support both. drop will be the first optional argument, to support backwards compatibility, and the second optional argument will be rotate. I'll enforce a rule that you cannot use drop and rotate simultaneously. I think that will cover all situations.

[bradleyharden]

If this PR is fully backwards compatible, then we won't need to consider a major revision yet. @LarsAsplund, do you have any idea when you will get a chance to review? I have pieces of #521 complete, and I would like to keep working on it, but I'm hesitant to do so without this PR accepted. It's a foundation for other changes.

[umarcor]

@umarcor, I suspect that drop was only ever added to integer_vector_ptr to mimic string_ptr, but string_ptr only has drop to support queue_t. I can't see many use cases for it.

If @LarsAsplund and/or @kraigher can confirm this, I believe that drop can be removed. Nonetheless, it would probably had beeen named p_drop if that is the case.

rotate is different than drop. It greatly simplifies resizing a circular buffer. I could explain more, but the code would be more precise.

I thinks it is important to have at least a sentence or a very simple diagram explaining it. I understand it as 'read n number of elements from the queue and drop them'. That's why I feel it to be a similar feature to drop. I don't think users should be required to read the code to understand what the procedure/function does.

I think the solution will be to support both. drop will be the first optional argument, to support backwards compatibility, and the second optional argument will be rotate. I'll enforce a rule that you cannot use drop and rotate simultaneously. I think that will cover all situations.

The point is that this can be more painful for users than just knowing that the implementation of drop has changed (although it is 'the same feature'), or replacing drop with rotate straightaway.

Is it worth adding this complexity in the source (increasing the effort of users that review the modifications) if it will be dropped in the next release?

If this PR is fully backwards compatible, then we won't need to consider a major revision yet. @LarsAsplund, do you have any idea when you will get a chance to review?

I think that this 'core' modifications are reviewed by @kraigher and not only by @LarsAsplund. In this sense, the issue that lead to #507 was started in april and the issue itself is pending since almost a month ago; it conflicts with this PR. I believe we should discuss how are we going to handle the merges, before rushing to it.

[bradleyharden]

I thinks it is important to have at least a sentence or a very simple diagram explaining it. I understand it as 'read n number of elements from the queue and drop them'. That's why I feel it to be a similar feature to drop. I don't think users should be required to read the code to understand what the procedure/function does.

To my knowledge, there is no existing documentation for the drop parameter, not even a comment in the package declaration. To understand and use the current drop parameter would require you to read the code. Does drop drop characters from the beginning or end of a string_ptr? There's no way to know without reading the code.

That being said, lack of documentation is still a fair criticism. I'll explain rotate. When resizing an integer_vector_ptr, you copy values from the old pointer to the new pointer. Normally, you begin copying at index 0. With the rotate parameter, you begin copying at an index equal to rotate. However, unlike drop, you do not stop copying when you reach the end of the old integer_vector. With rotate you continue copying values from the beginning of the integer_vector. Thus, the copied vector is equal to the old vector circularly rotated by rotate indices. I can add a comment to that effect in the package declaration, if that would help.

The point is that this can be more painful for users than just knowing that the implementation of drop has changed (although it is 'the same feature'), or replacing drop with rotate straightaway.
Is it worth adding this complexity in the source (increasing the effort of users that review the modifications) if it will be dropped in the next release?

I think you're misunderstanding my proposed solution. The implementation of drop would not change. The new resize functions would look like this:

procedure resize(
  ptr    : ptr_t;
  length : natural;
  drop   : natural := 0;
  rotate : natural := 0);

Current calls of resize that use drop must look like something like this

resize(ptr, 2**16, 125);

or this

resize(ptr, 2**16, drop => 125);

In either case, my new function will accept the existing call. In my new function, I will check if either drop or rotate is greater than zero. If exactly one of them is greater than zero, then I will implement the desired behavior. If both are greater than zero, then I will raise an error. I suppose you could define a way to use drop and rotate together, but that seems overly complicated to me.

This change to the PR would restore backwards compatibility. All existing code that uses drop would still work. There would just be a new option to use rotate, if desired.

[umarcor]

To my knowledge, there is no existing documentation for the drop parameter, not even a comment in the package declaration. To understand and use the current drop parameter would require you to read the code. Does drop drop characters from the beginning or end of a string_ptr? There's no way to know without reading the code.

I was not trying to criticize, but to sincerely ask a question the answer to which I was not sure about; even after looking at the code. That's why I tried to be proactive by proposing a kind of quoted description: 'read n number of elements from the queue and drop them'.

Regarding 'drop', I know it does discard the elements from the beginning, so I can give a valid answer to any other user asking about it. I just want to understand 'rotate' too.

I'll explain rotate. When resizing an integer_vector_ptr, you copy values from the old pointer to the new pointer. Normally, you begin copying at index 0. With the rotate parameter, you begin copying at an index equal to rotate. However, unlike drop, you do not stop copying when you reach the end of the old integer_vector. With rotate you continue copying values from the beginning of the integer_vector. Thus, the copied vector is equal to the old vector circularly rotated by rotate indices. I can add a comment to that effect in the package declaration, if that would help.

Thanks. Hence, given a buffer of eight positions:

 w
|0|1|2|3|4|5|6|7|
 r

PUSH `A`, `B`, `C`:
       w 
|A|B|C|3|4|5|6|7|
 r

POP (x2):
       w 
|-|-|C|3|4|5|6|7|
     r

ROTATE (x4):
       w
|4|5|6|7|-|-|C|3|
     r

I understand how this works when no resizing is involved. Moreover, I wonder if it makes sense to also provide rotate alone, with no resizing. However, this should probably be implemented by modifying tail/head, instead of copying all the values.

I also understand why this is used in e.g. resize(queue.data, 2 * size, rotate => tail).

I am not sure about what will happen if resize(queue.data, 1/2 * size, rotate => tail) is used. Is this expected? Or resize is always expected to grow? Once again, I'm just trying to get the context.

I think you're misunderstanding my proposed solution. The implementation of drop would not change. The new resize functions would look like this:

procedure resize(
  ptr    : ptr_t;
  length : natural;
  drop   : natural := 0;
  rotate : natural := 0);

Now that I understand the differences, I think that 'drop' can be a useful feature to have in a queue. But it should probably be unrelated to resize. In the end, drop is equivalent to calling pop multiple times and ignoring the values, or just increasing tail. Hence, it is easier to just add an optional 'drop' parameter to unsafe_pop or to make it a separate procedure.

Use case: a producer is saving packets of 16 elements in the queue, where the first element is used by a consumer to decide whether to process or ignore it.

resize(queue, size, drop => 15);
unsafe_pop(queue);

or

unsafe_pop(queue, drop => 15);

or

drop(queue, 15);
unsafe_pop(queue);

would allow to skip a packet and get the first item of the next packet.

In either case, my new function will accept the existing call.

I did the same for external modes.

In my new function, I will check if either drop or rotate is greater than zero. If exactly one of them is greater than zero, then I will implement the desired behavior. If both are greater than zero, then I will raise an error. I suppose you could define a way to use drop and rotate together, but that seems overly complicated to me.

What about this?

    procedure resize (
      ref    : natural;
      length : natural;
      drop   : natural := 0;
      rotate : natural := 0;
      value  : val_t := 0
    ) is
      variable old_ptr : va_t := storage(ref);
      variable new_ptr : va_t := new vec_t'(0 to length - 1 => value);
      variable min_length : natural := old_ptr'length-drop;
      constant offset : natural := rotate + drop;
    begin
      -- ??? min_length := minimum(length, min_length);
      if length < min_length then
        min_length := length;
      end if;
      for i in 0 to min_length - 1 loop
        new_ptr(i) := old_ptr((offset + i) mod old_ptr'length);
      end loop;
      storage(ref) := new_ptr;
      deallocate(old_ptr);
end;

[bradleyharden]

@umarcor, you seem to be confusing the queue and the underlying data storage (string_ptr before and integer_vector_ptr in this PR). There is no resize function for queue_t. Resizing of the queue is completely transparent to the user.

[bradleyharden]

@LarsAsplund, I restored the pool packages and the drop parameters. Everything in this PR should be 100% backwards compatible.

The current string of commits follows the discussion in this PR. If you would like, I can rewrite the history to make it easier to follow the changes from master. That might make it easier to review. Let me know which you prefer.

Also, before the final merge, I will need to rebase it onto the current master. I'm only one commit behind right now, but I don't see any sense in rebasing until you're ready to actually pull.

[bradleyharden]

Thinking more about queue pools made me realize I had a bug in queue_pkg.

Once again, this series of commits reflects events as they occurred. If you would rather I reformat the commits to better show the changes relative to master, just let me know.