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

CV32E40Pv2 verification update week 12 (bis) #2394

Merged

Conversation

XavierAubert
Copy link

Here is a small PR with mainly fixes made by @dd-baoshan and @pascalgouedo.

Just FYI @MikeOpenHWGroup, last time you asked about a path modification to riscvISACOV in 4a14542, commit a71ae14 restores it as we received a fixed version of the reference model.

Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me and it passes a local CI with Questa, so I will approve/merge.

Just FYI @MikeOpenHWGroup, last time you asked about a path modification to riscvISACOV in 4a14542, commit a71ae14 restores it as we received a fixed version of the reference model.

Thanks!

However, there are two items I am concerned about:

  1. Over use of $urandom() style system tasks.
  2. Use of illegal_bins. I opened Use of illegal_bins in the CV32E40P coverage model #2395 to discuss this.

@@ -66,10 +66,11 @@ class cv32e40p_rand_instr_stream extends riscv_rand_instr_stream;
bit idx_search_done = 0;
int rand_cnt = 0;

idx = $urandom_range(0, current_instr_cnt-1);
do begin
idx = $urandom_range(0, current_instr_cnt-1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan of using system tasks such as $urandom() because it makes writing constraints difficult. Having said that, I know that riscv-dv makes extensive use of these, so I will accept this.

@@ -421,7 +421,7 @@ class uvme_cv32e40p_zfinx_instr_covg extends uvm_component;
// from bhv_logic_3
cp_lsu_apu_contention_wr_rd : coverpoint cntxt.cov_vif.curr_rd_at_wb_regfile_wr_contention {
bins rd[] = {[0:31]} with ( (item < 32) & (fpu_latency == 1) );
illegal_bins rd_addr_32_63 = {[32:63]};
illegal_bins rd_addr_32_63 = {[32:63]}; // zfinx does not use freg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are declaring these as illegal_bins, does that imply a bug in the RTL, testbench or test-program? without looking at the logfiles, how will we know that one of these illegal bins has been hit?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This coverage file was created by previous owner and I only helped in fixing or update based on coverage analysis.
In my understanding, the illegal bin here mean for rtl issue if hit. It is more appropriate to have this behavior coded as rtl assertion but I did not to edit too much on this coverage model unless the illegal behavior is incorrect. I did mention this illegal_bin to Pascal and he is aware as well.

there are 2 ways to detect if illegal bin is hit. the simulation log (in this case, vsim) will issue runtime error in reporting. We can also observe illegal bin by open the ucdb for coverage analysis. However, we only interested on error reporting and not coverage analysis for illegal bin, therefore log file is the only way we use to detect such error.

Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let's keep this as-is for now. I will use Issue #2395 to suggest that we remove all illegal_bins and replace them with error assertions.

BTW, I really dislike parsing the simulation logs for for errors.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I really dislike parsing the simulation logs for for errors.

So do I 😀.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let's keep this as-is for now. I will use Issue #2395 to suggest that we remove all illegal_bins and replace them with error assertions.

BTW, I really dislike parsing the simulation logs for for errors.

@MikeOpenHWGroup , I agreed that every person has its own preferences but there are some points we need to think ahead how to address these runtime errors.
so far I know these are the sources of run time errors:

  1. assertion (immediate or property)
    • any assertion will print "Error" message in log file and Erorr count is increase in vsim log file.
    • we can implement the use of uvm_error by adding "else `uvm_error()" in the assertions clause.
  2. illegal bins in cover points
    • any hit will print "Error" message in log file and Erorr count is increase in vsim log file.

there are few things we need to take notes. illegal bins is allow according to systemverilog LRM. Even though we can set rules to prohibit the use of illegal bins in this tb, this will not become a full proof resolution because:-

  1. we cannot prevent third party packages from using it. e.g Imperas coverage model is using illegal bins for illegal instruction checks in its ISACOV.
  2. sometimes designers has tendency to code assertions in rtl files (note: here is referring to assertions that are not bind into rtl) and there is no obligation to prevent them from not using uvm_error because
    • using uvm_error macro requires import of uvm_package and macro included in rtl. (not all testbench is implement with uvm_methodology)
    • designers sometimes does not aware of uvm methodology and and these macro. they usually prefer using $error, $warning, or $fatal in assertions if necessary.

Copy link

@pascalgouedo pascalgouedo Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dd-baoshan for making the list of errors not managed by uvm_errors.
I could add to this list that Imperas model is not using uvm macros to report Hardware Loops constraints checks (as I understand it). But as they are already managing mismatch with those uvm macros, I guess HLC could do the same.

@MikeOpenHWGroup MikeOpenHWGroup merged commit cf56f21 into openhwgroup:cv32e40p/dev Mar 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants