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

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.

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.

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

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.

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

Okay, I am at it.

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

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.

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).

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?

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.

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.

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.