lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.49k stars 742 forks source link

[chip, dv] Incorrect recognized FSM #14966

Open weicaiyang opened 1 year ago

weicaiyang commented 1 year ago

This module has only combination logic. Not sure why VCS recognizes a FSM in this module and shows 0% FSM coverage in the report.

The coverage report for the upper module of the block can be found in this link, but if you click to see the detailed report of that block, it may crash as there is a huge generate block.

I'm following up this issue with AE.

cc: @msfschaffner

sriyerg commented 1 year ago

FYI, the report link appears to be broken.

weicaiyang commented 1 year ago

FYI, the report link appears to be broken.

oh, I shouldn't use the latest link. Changed to a fixed date report

weicaiyang commented 1 year ago

Copied AE's reply here. But I still don't know if this should be referred as a FSM. The logic in this module is to search the index of the max value on pure combination logic. I don't quite understand where can be inferred as the current state and next state. @msfschaffner @sriyerg Any thoughts?


Yes. I reached out to R&D. Here is their take:

assign vld_tree[Pa] = (sel) ? vld_tree[C1] : vld_tree[C0];

The above assignment combined with

assign sel = (~vld_tree[C0] & vld_tree[C1]) |

(vld_tree[C0] & vld_tree[C1] );

matches the definition of a one-hot FSM where each bit of vld_tree is a state and through “sel”, the value of each bit depends on another bit of vld_tree ( the next state of FSM variable depending upon current state of FSM variable).

Here is an even smaller example:

module top();

parameter int NumSrc = 32;

localparam int NumLevels = $clog2(NumSrc);

logic [2**(NumLevels+1)-2:0] vld_tree;

reg [NumSrc-1:0] valid_i;

for (genvar level = 0; level < NumLevels+1; level++) begin : gen_tree

localparam int Base0 = (2**level)-1;

localparam int Base1 = (2**(level+1))-1;

for (genvar offset = 0; offset < 2**level; offset++) begin : gen_level

          localparam int Pa = Base0 + offset;

          localparam int C0 = Base1 + 2*offset;

          localparam int C1 = Base1 + 2*offset + 1;

          if (level == NumLevels) begin : gen_leafs

              if (offset < NumSrc) begin : gen_assign

                         assign vld_tree[Pa] = valid_i[offset];

              end else begin : gen_tie_off

                         assign vld_tree[Pa] = '0;

              end

          end else begin : gen_nodes

              assign sel = (~vld_tree[C0] & vld_tree[C1]) |

                         (vld_tree[C0] & vld_tree[C1] );

              assign vld_tree[Pa] = (sel) ? vld_tree[C1] : vld_tree[C0];

          end

end : gen_level

end : gen_tree

endmodule;

If you do not want this FSM to be inferred, you could config VCS FSM extraction tool to exclude this FSM.

There is an easy way to do this. When you run the testcase with FSM coverage, a file called “fsm.verilog.generated_config.txt” gets created in the simv.vdb/snps/coverage/db/shape. You can use this file as the config file and edit it according to your needs. Copy this file to the current directory. Rename it as my_config.txt. If you rename it as cm.fsm_config, by default VCS checks for the presence of this file and excluded FSM from coverage based on the inputs on the file (cm.fsm_config). In this file you will see the details of the different FSMs extracted in every module. Include the following line in that FSM's detail, which you want the tool to exclude from coverage.

MODULE=top

FSMS=EXCLUDE

CURRENT=vld_tree

Rerunning the testcase with the config file as shown below will exclude the FSM from the report.

% vcs -cm fsm -cm_fsmcfg myconfig.txt test.sv

Here is some information on the FSM config file

https://spdocs.synopsys.com/dow_retrieve/qsc-t/vg/VCS/T-2022.06/vcs_olh/index.htm#page/Coverage_Technology_Reference_Manual%2Fcommand_ref.2.36.htm%23

sriyerg commented 1 year ago

Leaving this open until Synopsys provides a fixed version of VCS.

msfschaffner commented 1 year ago

Deferring into M3 since we have a workaround for this.

moidx commented 3 months ago

Moved to M5 as this is an issue to improve coverage.

andreaskurth commented 2 months ago

Suggesting to postpone this further to M7 as its part of V3 work