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

Better gated clock support #613

Closed
veripoolbot opened this issue Feb 5, 2013 · 8 comments
Closed

Better gated clock support #613

veripoolbot opened this issue Feb 5, 2013 · 8 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Feb 5, 2013


Author Name: Jie Xu (@jiexu)
Original Redmine Issue: 613 from https://www.veripool.org
Original Date: 2013-02-05
Original Assignee: Jeremy Bennett (@jeremybennett)


Hi all,

When simulating our design which uses gated clock, Verilator produced different simulation result compared with VCS. This causes our design can not be simulated correctly with Verilator. The test case is like this:


module sim_top (/*AUTOARG*/
    // Inputs
    clk,
    result
    );
    input clk;
    output wire [1:0] result;


    wire gated_clk;
    reg gated_clk_en;

    reg [1:0] pc;
    reg [1:0] res;

    initial begin
       pc     = 2'b0;
       res    = 2'b0;
       gated_clk_en = 1'b0;
    end

    assign result = res;
    assign gated_clk = gated_clk_en & clk;

    always @ (posedge clk)
    begin
        pc <= pc + 1;
        gated_clk_en <= 1'b1;
    end

    always @(posedge gated_clk) begin
        res <= pc;
    end
endmodule

While VCS will generally update signal res with the old value of pc, Verilator will always update res with the new value. Please have a look at the attached waveform for comparison.

I am not quite sure if there is a race condition in this case. Please help to comment.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Feb 5, 2013


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2013-02-05T14:11:50Z


More:

A search of "gated clock" in the Verilator forum indicates this problem has been here for a while. Based on some of the comments, I tried to use the /verilator clock_enable/ attribute to the gated_clk_en signal. And then the simulation result of Verilator is quite similar to that of VCS. Please find the attached waveform.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Feb 5, 2013


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2013-02-05T14:26:52Z


More:

While the /verilator clock_enable/ fix basically works with the test case, we can not use this in our design where there are a number of gated clocks and we are not supposed to change the RTL code at all.

It would be really nice if Verilator could behave the same as VCS in this context even without the clock_enable attribute.

In addition, the documentation about /verilator clock_enable/ and related IMPERFECTSCH warning could be more informative. For example, the IMPERFECTSCH warning maybe enabled by default and the introduction to IMPERFECTSCH warning could include sentences like "Ignore this "Ignoring this warning may make Verilator simulations differ from other simulators".

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Feb 5, 2013


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2013-02-05T14:30:48Z


To further illustrate this problem, I used synthesis tool to generate the gate-level netlist.

Please find the attached schematic view of the synthesized netlist.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Feb 5, 2013


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-05T15:39:28Z


Your IMPERFECTSCH note is a good one, I'll do that.

Unfortunately Verilator assumes the source code can be changed. It's possible to improve the command file syntax to separately identify the nets outside of source, I can give pointers if you want to prepare a patch to do that.

Otherwise changing the scheduling algorithm is a huge change that while certainly desirable isn't even on the schedule yet. If you or someone else is willing to put in several person months we can describe how to start the work.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Apr 9, 2013


Original Redmine Comment
Author Name: Jie Xu (@jiexu)
Original Date: 2013-04-09T14:53:04Z


Hi Wilson,

We were recently bitten by this problem hard, again. Therefore we think at least we would like to make some efforts to change this. Could you please help to point out where should we start? Thanks.
Jie

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jun 3, 2013


Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-06-03T17:40:30Z


Having worked with Jie on this, I believe it seems the problem is caused by the extra loop added for internally generated clocks by @processCircular()@ in @V3Order.cpp@. This is to deal with the race illustrated by the @t_clk_dpulse@ test. However I believe there is only a race where a clock is generated by delayed assignment, so the loop should only be added where the clock has been generated by delayed assignment.

With this change, two other tests, which pass with event driven simulators, but fail with Embecosm (@t_clk_condflop@ and @t_clk_powerdn@) now also pass with Verilator.

Please pull the patch from the circular-clocks branch at https://github.com/jeremybennett/verilator. This also adds a new test, @t_gated_clk_1@, based on Jie's example above.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Jun 6, 2013


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-06-06T03:38:01Z


Pushed to git towards 3.851.

Gave this a good tryout, seems stable. Good job debugging this; it's quite impressive you were able to get this down to a simple understandable fix with (hopefully) low risk of subtle side effects.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Aug 15, 2013


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-08-15T12:46:05Z


In 3.851.

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.