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

Statement queue pop_front error after foreach #1641

Closed
veripoolbot opened this issue Dec 15, 2019 · 13 comments
Closed

Statement queue pop_front error after foreach #1641

veripoolbot opened this issue Dec 15, 2019 · 13 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Dec 15, 2019


Author Name: Peter Monsson
Original Redmine Issue: 1641 from https://www.veripool.org


Hi Wilson,

Thank you for adding queues and associative arrays to Verilator. This is amazing!

I played around with it a little. The following code snippet in make_tracing_c:

int queue[$];
initial begin
   queue.push_front(1);
   foreach (queue[i]) begin
 $display("queue: %d", queue[i]);
   end
   queue.pop_front();

// queue.pop_back();
$display("ok");
end

Fails with this error:

In file included from Vtop__ALLcls.cpp:3:0:
Vtop.cpp: In static member function ‘static void Vtop::_initial__TOP__1(Vtop__Syms*)’:
Vtop.cpp:116:40: error: expected ‘;’ before ‘VL_WRITEF’
vlTOPp->top__DOT__queue.pop_front()VL_WRITEF("ok\n");
^~~~~~~~~
/home/pmonsson/verilator/include/verilated.mk:182: recipe for target 'Vtop__ALLcls.o' failed
make[1]: *** [Vtop__ALLcls.o] Error 1
make[1]: Leaving directory '/home/pmonsson/verilator/examples/make_tracing_c/obj_dir'
Makefile:59: recipe for target 'run' failed
make: *** [run] Error 2

This happens both with pop_front and pop_back after the foreach with the display. If the display inside the foreach is commented out, then I get no error.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-15T20:39:52Z


Hi,

from a very rough look at it, it seems that it does not like @queue.pop_front@ as standalone statement, but only in an assignment (using its return and not (void) usage): V3Width.cpp:2044-2053. This is at least a workaround, but I believe it is allowed to use it that way, right? Wilson, happy to solve that, its a part of the code I am eager to learn more about.

Cheers,
Stefan

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-15T20:42:03Z


Yes, if you could take a look that would be great.

For queues I didn't touch any code related for foreach which seems in retrospect suspicious as think should have been needed.

BTW with the display removed the optimizer will likely remove the foreach entirely which is probably why that makes it happy.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-15T21:06:48Z


Strangely, I don't observe that the error disappears when the display is commented out. It seems purely related to expecting to be an rvalue when it actually isn't for me.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-15T21:32:08Z


So, the issue itself seems to be easy: https://github.com/wallento/verilator/tree/issue-1641

As you mention, Wilson, the foreach seems to not work as expected it seems.

I am not sure about the syntax, but

queue.push_front(1);
queue.push_front(2);
foreach (queue[i]) begin
  $display("queue: %d", queue[i]);
end

is supposed to print

queue: 2
queue: 1

Do I understand that correct? I will look into it tomorrow then.

Cheers

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-15T22:39:28Z


I presume that's correct ([0] then [1] which contains 2 then 1). Also good idea to add test of foreach on an empty queue.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-15T22:49:53Z


Okay, I am at it.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 16, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-16T11:34:44Z


Hi, quick update:

  1. I found that @int q[0]@ is accepted and iterates @0:-1@. I found that other simulators don't accept @int q[0]@, while the standard doesn't explicitly say so in my opinion. Anyhow, this seems pretty useless. While the queue and this could maybe handled similarly, I propose to catch this case and also throw an error.

  2. While the general case is easy and solved, the empty queue is an issue (and identical currently to @q[0]@). It will iterate from 0 to -1 (both inclusive). This is due to the fact that during LinkParse the foreach is already replaced, but while the variable is not dereferenced. Hence it is impossible to make the differentiation here to catch it. Instead it now emits the code to iterate up/down and then with the limits, which are AttrOf at this point. In Width we resolve the AttrOf dimensions then and this is where I inserted code that will emit ldim=0 and rdim=.size()-1 for queues. So far so good, but the missing link is to catch size()==0. My proposal is to delay the lowering of AstForeach to a later stage, after the reference is resolved and then have the proper code emitted. What do you think?

Cheers,
Stefan

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 16, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-16T12:04:43Z


So, it seems that moving it from LinkParse to LinkDotResolve may make sense, with some minor tweaks.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-16T12:09:27Z


  1. The [$:0] means the maximum element is zero (not zero size, which IMO IEEE botched, anyhow). I think we should error on [$:-1]. If other simulators error on [$:0] (one element maxiumum) I'm fine with that.

  2. My comments about "empty" actually were referring to the dynamic state of the queue. The foreach needs to look at the dynamic, not static size, and if size() dynamically is zero must do nothing. I think this is a larger change? (It probably still means moving, I would think to V3Width, so it knows its a dynamic array).

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 16, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-16T15:54:32Z


Sorry, about 1. I meant standard arrays, not queues. I was checking them as drive by and an array with size 0 is accepted, but iterates incorrectly (identical to queue). I think it should be forbidden to have an array with size 0.

Yes, I am checking the dynamic size and the problem is in the code generated by the V3Width, because it will count from 0 to -1. I have been playing around now and it seems that as you write I must move the foreach->while transformation into the WidthVisitor. But I am now facing that I need to update my AST manipulation skills as I run into "unlinked" errors :)

btw: I don't receive redmine update mails anymore, is this intentional or an error on my side?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-16T17:46:43Z


I agree a zero size "normal" array, or a negative queue bound should be checked and flagged as an error. (As separate or combined patch from rest of this bug, whatever.)

I see redmine sending you mails and their being accepted by your mail server.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 17, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-17T08:18:07Z


The original issue here was "Allow queue operations to be used as statement". The first commit in the branch above solves it. I moved the other two issues that we discovered to #1642 and #1643, considering this issue itself solved.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 17, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-17T11:10:56Z


Peter, thanks for reporting, Stefan thanks for the great work.

Pushed (Stefan commit one) to git towards eventual 4.026 release.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.