8000 Fix toggle coverage of unitialized 4-state signals by RRozak · Pull Request #6072 · verilator/verilator · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix toggle coverage of unitialized 4-state signals #6072

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

8000 Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 38 commits into
base: master
Choose a base branch
from
Draft

Fix toggle coverage of unitialized 4-state signals #6072

wants to merge 38 commits into from

Conversation

RRozak
Copy link
Member
@RRozak RRozak commented Jun 9, 2025

Uninitialized 4-state signals have value x. Currently, if the first value assigned to such a signal is 0, the coverage counter is not increment. However, it is incremented if the change is from x to 1. I think the incrementation should be skipped in both cases.

This draft PR contains WIP implementation of the following approach:
Each bit of a 4-state variable has 2-bit variable associated. It may have the values 0, 1, 2, which have the following meaning:

  • 0 - the original variable is uninitialized
  • 1 - the original variable is initialized, but don't increment the counter yet
  • 2 - the original variable is initialized and increment the counter

We change the value from 0 to 1 after the first assignment to the original variable. That assignment takes place before the coverage counter is incremented so that's why we need the value 2. If the value is 1 when the condition of incrementation of the counter is tested, we change the value to 2 instead, to make it incremented after the next change.

The current implementation is not completed yet. I created the PR to discuss the approach.

TODO:

  • make it work for other types than 1-bit logic
  • skip variables initialized in the declaration
  • skip variables driven by top module input ports

I also had a problem with optimizations. The assignments to initp variables were removed in V3Gate, because the reading of their values is added in V3Clock.

b-chmiel and others added 30 commits June 9, 2025 11:24
@b-chmiel @RRozak
Failing test
94fbdbb 
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
@b-chmiel @RRozak
WIP: add Vtogcovinit for tracking whether initialization happened
cee15b9 
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
@RRozak
Change bit to logic
6e27371 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
WIP: Add InitVar only if type is fourstate
e13b6b4 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Pass initVarp only if not null
890c0dc 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Handle nullptr initp in V3Clock
90dca9c 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Make initp field 8000 optional
9459061 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add p suffix
f401743 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Delete initRefp in cleanup and add comment
5923627 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Don't increment coverage in 1st assigment to 4-state var
f0747b1 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Throw assert on non-opaque type
57a10ed 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
TMP: Throw fatalSrc if initp is not a varref
579c068 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add initialization and change VAccess
0d764c3 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Mark as init only when values change
ddb5eef 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
8000 Make init var int type
0d13399
@RRozak
Increment counter if initVarp is 2. If 1, set it to 2
a29bd3c 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add new dtype to type table
3be0b11 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Clone varref when adding for 2nd time
880d838 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add assignment to init var
bdf0450 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Set initVarp to 1 only if it is 0
7dacd7a 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Change initVat to 2 if it is 1
e8e8e1f 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Fix CHECK_COVER offsets
efe37b2 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add initial block and run simulation for 10 cycles
3f9f853 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Rename test
b7844be 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Remove cases with initialized var
b7f6627 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add bit variables
620a58a 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Replace asserts with conditions
5b19a91 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Fix checking if type has ranges
353b328 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Skip AstCoverToggle in V3Delayed
b285c7e 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Skip warnings
8d49cf9 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
RRozak added 8 commits June 9, 2025 11:24
@RRozak
Disable coverage of initVarp statements
09cf54a
@RRozak
TMP: Add edges in both directions in V3Gate if varref is child of Cov…
7377320 
…erToggle

Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Replace assignments with always
e0abf4b 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Remove initVarp initialization
dd9a073 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Change type to bit
977b1cf 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Update number of optimizations
a573ba9 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Skip top module input ports
64f18f2 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Change comments
24e61af 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@wsnyder
Copy link
Member
wsnyder commented Jun 9, 2025

I'm worried about the runtime cost here. Usually the practice is to wait for reset, which should clear all user signals, then clear all toggle counters. Does that solve most your concern?

@RRozak
Copy link
Member Author
RRozak commented Jun 9, 2025

I don't think it solves our problem.
The reset signal may be set in one test a few times and a user wants to know the coverage of the whole test, not only the coverage after the last reset. A design may also have more than one reset signal.

@wsnyder
Copy link
Member
wsnyder commented Jun 9, 2025

Usually there is still some init-done point, and even if not, I dont understand how this change prevents false coverage during first reset.

@RRozak
Copy link
Member Author
RRozak commented Jun 9, 2025

This change only prevents from incrementing the coverage counter on the 1st assignment to the variable which has the default value x. It isn't related to reset signal.

@wsnyder
Copy link
Member
wsnyder commented Jun 9, 2025

My point is from the verification engineer's standpoint, proving something has toggled is related to reset - you can only ensure functionality of a toggle outside reset, so they should zero coverage for all time before reset/init is complete. I also assume that reset clears out critical X's. With both those methodologies I think this change isn't needed.

@RRozak
Copy link
Member Author
RRozak commented Jun 10, 2025

Cocotb tests do many resets. They run a single Verilator binary and run multiple tests on it. Each test typically starts with a reset. So if we zero the counters, we'll loose coverage metrics.

What do you think about making this feature optional and disabled by default to not slow down simulation for users who don't need it?

@wsnyder
Copy link
Member
wsnyder commented Jun 10, 2025

Sorry I was not clear it is only the first init that is not part of any test resets does the clear. Later resets are covered intentionally, so that you can see toggles on resets. Note this means that to see reset in toggle coverage, the test must be out of reset, then do a reset, which is intentional as otherwise resets have not been well proven.

@RRozak
Copy link
Member Author
RRozak commented Jun 11, 2025

I am not sure if I understand. Do you mean adding an option to zero counters once initial blocks are evaluated?

628C
@wsnyder
Copy link
Member
wsnyder commented Jun 11, 2025 •
edited
Loading

A methodology should, for tests using any sort of coverage (not just toggle) ,in pseudo-code, do roughly:

initial
    start clocks
    do reset sequence
       couple cycles
       assert reset
       run few cycles
       disable reset
       run few cycles
    // note above should clear out "X"s on any control state if using a 4-state simulators,
    // in Verilator will clear out any x-rand-resets on control state
    enable_assertions
    clear_coverage  ($c("VerilatedCov::clear()")  -- or we add $assertcontrol support)
    run_a_test
       test might also do a reset as part of testing
    write_coverage

@RRozak
Copy link
Member Author
RRozak commented Jun 12, 2025

We want to implement a solution that will not require the user to modify the code, so that they can switch easily from another simulator to Verilator. Manually inserting $c("VerilatedCov::clear()") doesn't solve our problem.

@wsnyder
Copy link
Member
wsnyder commented Jun 12, 2025

We want to implement a solution that will 72DA not require the user to modify the code, so that they can switch easily from another simulator to Verilato

That's fair, implement the standard $coverage_control(SV_COV_RESET)`. You could skip for now other controls and providing a hierarchy. I thought there was a bug on that, but looks not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RRozak @wsnyder @b-chmiel
0