8000 Use separate coverage counters for changes 0 -> 1 and 1 -> 0 by RRozak · Pull Request #6086 · verilator/verilator · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use separate coverage counters for changes 0 -> 1 and 1 -> 0 #6086

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 Git 8000 Hub”, 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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Use separate coverage counters for changes 0 -> 1 and 1 -> 0 #6086

wants to merge 7 commits into from

Conversation

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

It creates separate toggle coverage counters for changes from 0 to 1 and from 1 to 0.

The counter which should be incremented is chosen by the current value of the bit to which it corresponds. I decided to add the expression which evaluates to the value of that bit to AstCoverInc and then emit it as part of index expression of vlSymsp->__Vcoverage.
We can store the expression in AstCoverInc at the beginning, but it is the same expression which is stored in AstCoverToggle, so I decided to store it only in the latter. When AstCoverToggle is removed, I copy it to AstCoverInc.

RRozak added 7 commits June 11, 2025 08:52
@RRozak
Add isToggle field
b9169e1 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Reserve 2 counters for one toggle coverage point
71d0494 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Pass expr from AstCoverToggle to AstCOverInc
140e1f6 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Increment coverpoint given by value of bit
a02117e 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
InsertClean to AstCoverInc expr
7c4e963 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Update tests
7fd0729 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@RRozak
Add comments
b8d072f 
Signed-off-by: Ryszard Rozak <rrozak@antmicro.com>
@wsnyder
Copy link
Member
wsnyder commented Jun 13, 2025

Do other simulators likewise capture 0->1 and 1->0 separately? At my site we just look for e.g. 10 transitions, and then know both 0->1 and 1->0 are well covered. This change also increases runtime, but I'm guessing other simulators do it.

t/t_cores_veer_el2_cmark.py --coverage-toggle
( cd ./obj_vlt/t_cores_veer_el2_cmark ; time ./obj_dir/Vtb_top --test-halt )
  • Baseline: 5.331s
  • --coverage-toggle pre-change: 6m41.268s (yes, really x slower)
  • --coverage-toggle this change: 6m40.107s (yes, really x slower)

I don't understand how this can be faster then the old code, as there's an additional operation, so probably some unintentional artifact, but point is it's small.

For multibit signals I think we'd be better off making a function, this will likely help performance a lot. Where you had:

     if ((4U & (sig ^ (IData)(__Vtogcov__sig)))) {
         ++(vlSymsp->__Vcoverage[20 + (1U & (sig >> 2U))]);
         vlSelfRef.__Vtogcov__sig = ((0xf7U & (IData)(vlSelfRef.__Vtogcov__sig)) | (4U & sig));
     }
     if ((8U & (sig ^ (IData)(__Vtogcov__sig)))) {
         ++(vlSymsp->__Vcoverage[22 + (1U & (sig >> 3U))]);
         vlSelfRef.__Vtogcov__sig = ((0xf7U & (IData)(vlSelfRef.__Vtogcov__sig)) | (8U & sig));
     }

I suggest we do for multibits:

     if (sig ^ __Vtogcov__sig)  {  // Any change
         VL_COV_TOGGLE_CHG_I(4 /*width*/, 9 /*counter#*/, sig, __Vtogcov__sig);  // Also will need _W flavors
         // Note this will update 9..{9 + width}
         vlSelfRef.__Vtogcov__sig = sig;   // Copy every bit
     }

This may be slightly slower if only one bit changes in a bus, but faster if many bits change. Anyhow proof will be in the benchmarking.

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

I noticed that candidate for improvement. The same situation is currently on master. Values of Vtogcov variables are updated bit by bit. I don't know how hard is to change it. It is a result of V3Coverage, which creates separate AstToggle* nodes for each bit.

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

I know that at least one popular simulator captures 0 -> 1 and 1 -> 0 separately. I also found an article for Aldec Active-HDL which shows separate coverage for 0->1 and 1->0: https://www.aldec.com/en/support/resources/documentation/articles/1511.

Also, 10 transitions don't necessary mean that transitions in both directions took place. It isn't true if we have 10 instances of a module which contains only 0->1 transitions.

@wsnyder
Copy link
Member
wsnyder commented Jun 16, 2025

Fair points. Note Verilator captures numbers separately for the instances, it's the annotation that might combine them.

@wsnyder
Copy link
Member
wsnyder commented Jun 16, 2025

Anyhow I'm ok taking the change but would like the refactoring mentioned to reclaim performance, it can be part of this patch or a separate pre-pull (which has the advantage the tests shouldn't need to change).

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.
2 participants
@RRozak @wsnyder
0