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

Verilator ignores sense list of combo always block #757

Closed
veripoolbot opened this issue May 2, 2014 · 6 comments
Closed

Verilator ignores sense list of combo always block #757

veripoolbot opened this issue May 2, 2014 · 6 comments
Labels
area: performance Issue involves performance issues area: scheduling Issue involves scheduling/ordering of events effort: days Expect this issue to require roughly days of invested effort to resolve resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@veripoolbot
Copy link
Contributor


Author Name: Jie Xu (@jiexu)
Original Redmine Issue: 757 from https://www.veripool.org


When investigating to improve the performance of Verilator-based simulator model, I found the following handling of combo blocks in Verilator:

  1. The sense list in always combo block is actually not really used. When forming AstActive, the sense list of combo always block is deleted.

  2. Instead Verilator will kind of take in all the AstVarRef on RHS as the sense item. This is reflected in V3Order.cpp when forming the graph. This in some cases will make the signal as Circular.

We have some cases where the above of handling will make Verilator generating at least inefficient(not know if incorrect) code. For example:

module test (a, b, result);

input  wire       a;
input  wire       b;
output wire [31:0] result;

logic [2:0] c;

always @(a,b)  // By doing this, the designer knows c is not a problem.
begin
// c = 3'b0;  // by uncomenting this line, Verilator will not take c into sense list.
c = c | (a << 1);
c = c | (b << 2);
end

assign result[2:0] = c;
endmodule

Verilator will take the @c@ into the @change_request@ list in this example. However here @c@ is an output and don't really need another loop to settle.

The big question here is which should we depends on, either the designer's list (@Always(a,b))@) or all the VAR used as condition. IMHO, I believe we should respect the designer as he has the option @Always*@ if he really want to take later case.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-05-02T12:19:40Z


Verilator follows the synthesis rules which you described - basically treat all combos as an "always @*".

Obviously Verilator is cycle based, so will not do what the sensitivity list strictly requires; though it could be told to use only the list for doing ordering (V3Order). The main catch is there are things that need to be sensitive to for proper scheduling that are not legally or traditionally listed in the sensitivity list, for example memories or the variables that a function call inside the block needs.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2014-05-02T12:24:50Z


Agree with your points. But it would be nice to have an option to Verilator which determines how the sense list should be constructed. The default process can still be "always @*". Then when user uses that option, he should be responsible for what is he doing.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-05-02T12:29:03Z


I'm fine with adding an option like that. My caution is that you will need to solve the problems of those items which cannot be legally put into the sensitivity list.

It would require changing the verilog code, but one option would be to add a meta comment (that becomes an attribute after parsing):

always @(a,b)  // By doing this, the designer knows c is not a problem.
  begin
  // verilator strict_sense_list
  // c = 3'b0;  // by uncomenting this line, Verilator will not take c into sense list.
  c = c | (a << 1);
  c = c | (b << 2);
end

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-05-02T12:32:59Z


I didn't mention the point of the meta comment is you would use it only when there are no function calls or other stuff that might get you in trouble, then you don't need to solve that issue.

Both methods are also probably reasonable, add the switch and it would have the same effect as setting that meta comment on all combo always'.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2014-05-02T12:40:44Z


The reason I want a switch is that it is quite difficult to find all the cases in a relative large design. And this is really important one as it may affect both the simulator performance and accuracy.

I actually have tried another solution as well. I add meta info @/verilator clock_enable/@ to "c". That also works in the sense eliminate the extra loop. However, when I tried this trick in our larger design, then the simulator behaved not correct any more.

So for the meta info method, my concern is the design will not simulate correctly if we only change some of the always blocks.

@veripoolbot veripoolbot added area: performance Issue involves performance issues area: scheduling Issue involves scheduling/ordering of events effort: days Expect this issue to require roughly days of invested effort to resolve type: feature-IEEE Request to add new feature, described in IEEE 1800 labels Dec 22, 2019
@wsnyder wsnyder added the resolution: fixed Closed; fixed label May 15, 2022
@wsnyder
Copy link
Member

wsnyder commented May 15, 2022

Believed fixed in develop-v5 with #3278.

@wsnyder wsnyder closed this as completed May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: performance Issue involves performance issues area: scheduling Issue involves scheduling/ordering of events effort: days Expect this issue to require roughly days of invested effort to resolve resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Development

No branches or pull requests

2 participants