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

unable to unroll for loop causing BLKLOOPINIT error #677

Closed
veripoolbot opened this issue Sep 13, 2013 · 10 comments
Closed

unable to unroll for loop causing BLKLOOPINIT error #677

veripoolbot opened this issue Sep 13, 2013 · 10 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Sep 13, 2013


Author Name: Jie Xu (@jiexu)
Original Redmine Issue: 677 from https://www.veripool.org
Original Date: 2013-09-13


When compiling our RTL code with Verilator, we got "BLKLOOPINIT" errors. According to Verilator manual, we try to increase the @--unroll-count 256@ and @--unroll-stmts 99999@. However, still Verilator reports the "BLKLOOPINIT" errors.

After a bit investigation, we found the root of the error actually is that Verilator is unable to unroll the for loop which has a little bit complicated condition, e.g. @for (i=0; (i< 4) && (i > 2); i++)@.

This means the error message in this case is not quite correct since it actuall report

Unsupported: Delayed assignment to array inside for loops (non-delayed is ok - see docs)

A test case to regenerate the issue is attached.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 14, 2013


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-09-14T12:22:38Z


Yes, the unroller is pretty stupid, it predates the internals being able to emulate most code.

Perhaps you might be willing to look at providing a patch? Basically if you look at V3Unroll now it just looks for certain hardcoded constructs. Instead it should apply values in the initial part of the for, then execute the test and increment, and 'timeout' if too many loops are needed, or anything strange happens (like other variables are needed, or an unsupported construct like PLI call is seen). V3Table has good examples of how to execute code during compilation.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 14, 2013


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2013-09-14T13:40:57Z


I think I could give it a try. Will update you if there is some progress or problems.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 5, 2015


Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-10-05T15:51:36Z


Wilson,

I wrote up an initial version of this over the weekend, it's mostly passing tests but needs some more work. I'm somewhat worried it'll have a negative performance impact though, especially for the cases where we decide to not unroll the loop. Previously it was a fairly quick check, but now we either have to

  1. run the simulation for the incr/condition to determine size of loop, then bail if too big
  2. Run the actual unrolling and then abort if it goes for too long.
    both which seems fairly slow.

Is there any performance benchmarks I can run to ensure things are looking good after this change?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 5, 2015


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-10-05T16:31:17Z


I don't have any verilator-compiletime benchmarks. I assume you have some fairly large jobs you could just "time" - just make sure your machine is idle otherwise for fairness. I suspect it will be in the noise, but good to check.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 6, 2015


Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-10-06T03:03:57Z


Thanks. I've pushed a draft to https://github.com/phb/verilator-asserts/tree/#�, please let me know what you think.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jan 19, 2016


Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-01-19T01:12:55Z


Revived this patch here: https://github.com/phb/verilator-dev/tree/#�
The patch also fixes #�

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jan 21, 2016


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-01-21T02:55:55Z


Sorry I missed providing feedback on this.

  1. Nit, it's "if (...)" not "if(...)". There's no space after function names "fileline()" not "fileline ()". These match GNU conventions (in theory).

  2. forUnroller now seems to return a bool indicating additional "did it" status, but this isn't tested in forUnrollCheck. Presumably it should be, otherwise the loop will get mis-deleted. (Try #3 below it might find this.)

  3. I think you have them all covered, but to the test cases, please add a complicated non-simulatable thing to init, cond, and inc, for example

    j = 0; for (i=$c32("1"); i<3; ++i) j++;
    if (j!=2) $stop;
    j = 0; for (i=1; i<$c32("3"); ++i) j++;
    if (j!=2) $stop;
    j = 0; for (i=1; i<3; i=i+$c32("1")) j++;
    if (j!=2) $stop;

  4. I get a warning:

../V3Unroll.cpp:200:11: error: unused variable 'proceed' [-Werror=unused-variable]

In your .bashrc or whatever shell equivelent please

export VERILATOR_AUTHOR_SITE=1

then reconfigure and build emacs, so you get warnings.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jan 21, 2016


Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-01-21T03:51:08Z


Thanks for the review. Updated the patch with the additional tests and fixed the issues you mentioned. https://github.com/phb/verilator-dev/tree/#�

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jan 22, 2016


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-01-22T00:01:54Z


Great! Fixed in git towards 3.881.

@veripoolbot veripoolbot closed this Mar 2, 2016
@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Mar 2, 2016


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-03-02T00:15:55Z


In 3.882.

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
1 participant
You can’t perform that action at this time.