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

Foreach of dynamic sized queue #1643

Closed
veripoolbot opened this issue Dec 17, 2019 · 4 comments
Closed

Foreach of dynamic sized queue #1643

veripoolbot opened this issue Dec 17, 2019 · 4 comments
Labels
area: data-types Issue involves data-types effort: days Expect this issue to require roughly days of invested effort to resolve resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1643 from https://www.veripool.org

Original Assignee: Stefan Wallentowitz (@wallento)


Foreach on queues is not supported, see #1641

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-17T21:10:10Z


Hi,

in the end it was easier than I originally experimented: Fixing the attribute calculation (DIM_LEFT, DIM_RIGHT) for queues to be calculated at runtime (or constant 0 for DIM_LEFT) solves the foreach already. Unfortunately, the emitted code does not work for empty queues then, because DIM_RIGHT is -1 (as per the standard and size()-1 calculation). I first went for a complex solution where the foreach code was generated later, but that lead to many other issues. So, what I ultimately decided for is to add a check for the size before checking the iteration limits: It now checks if the array size is greater than zero and ANDs that with the countup/countdown limits logic. There is no impact on other arrays as it will be removed (constant true), but there is a probably a small impact on the code size for queues as it will have this extra check in the code. But this is okay I think.

You can find the patches here: https://github.com/wallento/verilator/tree/issue-1643

  1. 547bb35: Fix dimension calculation for queue, string, struct
  2. 62669e2: Fix AttrOf for queues, #�
  3. ae1fa15: Generate foreach in a way that supports queues, #�
  4. 96d0369: Improve tests for queues, #�

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-18T00:34:01Z


I don't understand your DIM_RIGHT comment; I would have thought a for(0..-1) would not iterate, but anyhow looks like you got something working.

I can't compile this, think you need a "default: nodep->v3error("Unhandled attribute type") to solve this:

../V3Width.cpp: In member function 'virtual void WidthVisitor::visit(AstAttrOf*)':
../V3Width.cpp:1082:24: error: enumeration value 'ILLEGAL' not handled in switch [-Werror=switch]
              switch (nodep->attrType()) {
                     ^

(Maybe you don't have VERILATOR_AUTHOR_SITE=1 in your .bashrc file so it's there when you ./configure?)

Anyhow logically looks fine to merge once that's fixed.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-18T05:19:46Z


When @$left(array)=0@ and @$right(array)=-1@ the emitted code will iterate @foreach(array[i])@ as @i=0@ and @i=-1@. This is the case for an empty queue, but also for @bit [0:-1] array@, but only in the latter case it is expected behavior. Hence, checking for @$size(array)@ before seems the only way to differentiate them both as it is 0 for the empty queue but 2 for @bit[0:-1]@.

Ah, I didn't know this env variable, thanks! Indeed the switch-case is not complete but the values are actually not reachable as it is embedded in another switch-case of those values used. But its more robust of course and I have amended and force pushed it. Do you know an elegant way to have it fail at compile time? Something to detect if it becomes reachable code would be nice. Thanks!

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-18T23:33:52Z


Thanks for the work, pushed to git towards eventual 4.026 release.

The incomplete switch messages are automatic from gcc ~7.4.0 and newer AFAIK.

@veripoolbot veripoolbot added area: data-types Issue involves data-types effort: days Expect this issue to require roughly days of invested effort to resolve reolution: fixed resolution: fixed Closed; fixed and removed reolution: fixed labels Dec 22, 2019
@wsnyder wsnyder closed this as completed Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: data-types Issue involves data-types effort: days Expect this issue to require roughly days of invested effort to resolve resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

2 participants