Multiple comma-separated declaration in one for-loop
In the SystemVerilog standard, there can be multiple comma-separated in one for-loop declaration like this.
for (int count = 0, done = 0, j = 0; j * count < 125; j++, count++) $display("Value j = %d\n", j );
However, it is not supported by Verilator, at least in 4.004. Is there any plan about supporting this now? Or, can someone provide some instructions about how to write a patch to support this.
RE: Multiple comma-separated declaration in one for-loop - Added by Wilson Snyder almost 2 years ago
It would be excellent to have improvements here.
The reason this is the only format supported is the loop unroller at present only recognizes this form, so performance will die otherwise. So I'd suggest two sets of patches.
First, improve V3Unroll to not look at the initializer list and instead try to expand every loop.
Then with that done you can change the verilog.y to take all statements.
Hello, I find supporting this is difficult since I have little experience about compilers. I try to work on this but so far I can only make a patch which can detect the comma inside a for-loop. https://github.com/johnjohnlin/verilator_fork/commit/3483c3291f3ae1d33037b43458e47aad31c27d1cAfter
%Warning-USERWARN: a.sv:3: Initialization after the first comma is ignored now %Warning-USERWARN: Use "/* verilator lint_off USERWARN */" and lint_on around source to disable this message. %Warning-USERWARN: a.sv:3: Assignment after the first comma is ignored now %Error: Exiting due to 2 warning(s) %Error: Command Failed /opt/verilator_dev/bin/verilator_bin --cc a.sv
So I want to ask for some clues about the development. I have inspect the Bison code, and it seems that "for_initialization" returns two types of linked-list (am I correct?).
%Error: a.sv:3: syntax error, unexpected ',' %Error: Cannot continue %Error: Command Failed /usr/bin/verilator_bin --cc a.sv
Therefore, is it reasonable to change the structure like this?
With type: AstVar->AstAssign->nullptr No type : AstAssign->nullptr
With type: AstVar->AstAssign->AstVar->AstAssign->...->AstVar->AstAssign->nullptr No type : AstAssign->AstAssign->...->AstAssign->nullptr
Finally, you said that I must improve V3Unroll as well, but I cannot understand how this code interact with the data-structure above. Can you also provide explanation about this? Thanks.
I applied code similar to your patch to trunk as it seemed a good general improvement. I changed it slightly to match the IEEE grammar and Verilog-Perl's implementation.
As to next steps, you would change where now it prints the error to do '$$=$1; $1->addNext($3);' then if you look at the .tree outputs the AstAssign or AstVar will have a next() that points to the following assignments. Then in V3Unroll handle them, and in later steps if not unrolled you can move those statements to the new format (before or inside the AstWhile that replaces the AstFor.)
I implement the basic version for this feature based on v4.008. Current code is changed significantly since later assignments can depend on previous ones for (e.g. i = 1, k = N-1-i; see below), which is not very compatible with current code logic. (My understanding is that, in the current code, the Unroller computes one loop-variable, concatenates the loop-body and loop-increment, then replaces all rvalue occurrences of that loop-variable to to a AstConst.)
As a result, I change two things into vectors, including loop-variables, ignored assignments. Also, the assignment is simulated one-by-one now, and the variable update is executed immediately after every RHS update. I use the following example to demonstrate that my implementation basically works. I attach the generated node trees from the code below. For the for-loop with only one variables, the new Unroller generates the same tree as v4.004. For the for-loop with two variables, I manually inspect the generated tree and it seems good. However, since a buggy Unroller can be devastating, this commit still should be reviewed and cleaned-up carefully.
BTW, the new rules in v4.010 seem to accept something like this (loop-variables with different type):
module MODULE#(parameter N = 3)(input [N-1:0] x, output [N-1:0] y); `ifdef NEW always_comb begin for (int i = 0, k = N-1-i; i < 3; i++, k = N-1-i) begin y[i] = x[i]; end end `else always_comb begin for (int i = 0; i < N; i++) begin y[i] = x[N-1-i]; end end `endif endmodule
, but this is not a legal in C/C++ so I comment them out in this commit.
for (int i = 0, double k = N-1-i; i < 3; i++, k = N-1-i) begin
MY_TEST.zip (206 KB)
Thanks for this work, I need to spend some more time looking at the details of this, but for now I think the big thing would be to see if we can get the 4.010 multiple inits working again, and if you could please add new test_regress tests to your branch that try the new features. Thanks.
I add 2 small files for this and this regression works fine. Also, t_for_comma_bad regression can be removed later. https://github.com/johnjohnlin/verilator_fork/commit/5e01839c5d4fecd801645eff558d512366fdfe1f
Sorry, think this got lost, I suggest:
1. File a bug so I don't forget :)
2. The code needs to support the multiple inits.
3. Need to rebase.
4. An alternative might be to have the patch a bit more generally record whatever variables are in the init section only. Then track these through v3simulate replacing each as it goes, if the loop terminates after a reasonable iteration count then do the real expansion. The advantage of this is the loop condition and post-increment can basically be normal code - no need for checking bounds etc any more (most of that algorithm was from before V3Simulate existed).