nickg / nvc

VHDL compiler and simulator
https://www.nickg.me.uk/nvc/
GNU General Public License v3.0
631 stars 77 forks source link

WIP: Temporary coverage rework. #865

Closed Blebowski closed 5 months ago

Blebowski commented 6 months ago

Hi,

this is the MR you were asking about in the conversation. But it is in a very "raw" state...

The ideas are following:

  1. Make single cover_item_t for a single coverage bin. Convert all the coverage run-time data to counters (since single cover_item_t will represent single bin, no masking and shifting is needed). This will simplify handling of coverage items, coverage merging, etc. Maybe VCODE OPs could even be reworked to have only a single VCODE OP type (coverage OP) with some sort of sub-kind that. The lower logic would change to emit new block that is conditionally executed/skipped if the given condition/branch/expression is evaluated to true. The execution code for each Coverage op will be just adding +1, so no complex handling will be needed in the JIT layer for different coverage OPs. Basically everything will be the same as Statement coverage is now. The name of the "BIN" would then be suffixed to the name of the emited cover_item_t. The coverage exclude logic will then no need to specify any bins, everything will be simply encoded in the name of the coverage item. This is where the user interface would change slightly.

  2. Consecutive "N" coverage items (that are now encoded in single item) will be still reported in a single "table" in the report. To do that, one would need to identify how many consecutive items belong to "the same construct". The num attribute of first such item can be used to identify how many next consecutive items are to be grouped. It is already done like that for FSM coverage. The num identifies all cover items to be put into single table since they belong to single FSM. This can be generalized for all coverage items.

  3. Simplify interface between lower and cover parts. Move all the logic that emits the coverage items into cover (e.g. as is now done for the toggle coverage). The cover_add_item would change to cover_add_items and return pointer to first coverage item added. Since num would contain number of consecutive items, it would be simple to iterate these items in lower and create the branching logic needed to execute/skip the VCODE cover op. All the API functions like: cover_inc_array_depth, cover_dec_array_depth, cover_skip_array_toggle cover_skip_type_state and cover_get_std_log_expr_flags would be now only local to cover part.

Possible implications:

nickg commented 5 months ago

I split out just the branch coverage changes from this in 08505635e929d3.

Blebowski commented 5 months ago

Thanks, doing this step-by-step is much better approach than all at once.

Blebowski commented 5 months ago

I think it makes sense to close this MR, throw away the branch as dead and do the changes step-by-step. I tried to give it couple of hours and I got lost. I ran into the pitfall of trying to fix everything at once...