ISACOV : incorrect passed arguments in get_instr_value_type() method

[AyoubJalali]

Hello @JeanRochCoulon @ASintzoff , So I have notice some incorrect argument value in get_instr_value_type(bit[XLEN-1:0] value, int unsigned width, bit is_signed) for some C extension instruction,starting from https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L232 to https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L279 , the problem is in width argument, because the length given don't match with the length of the imm field extract using the get_field_imm() method https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_instr.sv#L374 , but I think it match perfectly with get_data_imm(), so i propose to change the get_field_imm() with get_data_imm() in the ISACOV monitor, or we correct the width values to match with the get_field_imm(). Some of this instruction : https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L234 https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L238 https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L244 https://github.com/openhwgroup/core-v-verif/blob/9962aebe36b339bf88ca7e1d8cb3143e10edbda4/lib/uvm_agents/uvma_isacov/uvma_isacov_mon.sv#L250 ...

[MikeOpenHWGroup]

Hi @silabs-robin, can you have a look? AFAICT, you are the author.

[silabs-robin]

Nice catch. It is a bug indeed. For example, c.swsp has a 6-bit immediate, but the quoted code is using 8 because there are 2 implicit zero-bits, and your are right about the get_data/field_imm() difference.

For how to fix it, I am not sure which alternative is better. Preferably, the one that is the most likely to reveal errors. But I don't know which of them that would be, so I'll trust whichever decision you make :)

This relates to what I was partially fixing one year ago (that "RVC" was not done correctly in isacov). Though I can't remember if I have any reason to suspect the non-compressed 32-bit instructions, I know for a fact that RVC still had problems when I left it. I have one method for verifying isacov correctness (detailed below) but I am not sure if it would have caught this particular problem (I doubt it).

Question: @AyoubJalali , how did you spot this? Were you investigating missing coverage, manually reviewing the code, adding new features, or something else?

---

The "tool" I used to help in checking isacov correctness is a make target called isacov_logdiff. The idea is to let isacov dump a log file of everything that it has processed, using the data types that that it has parsed, then compare this with the rvfi log to see if everything agrees.

Here is an example. (I don't know how much the cva6 environment differs from 40x/s, but this illustrates the principle nonetheless.) First I enabled isacov logging isacov_cfg.trn_log_enabled == 1 and ran a test make test TEST=hello-world COV=1. Then I ran the diffing make isacov_logdiff TEST=hello-world. In my case it outputed:

isacov_logdiff:
checking that env/dirs/files are as expected...
extracting assembly code from logs...
world/0/uvm_test_top.env.isacov_agent.trn.log
coveragelog /work/ropeders/src/core-v-verif/cv32e40s/sim/uvmt/xrun_results/default/hello-world/0/uvm_test_top.env.isacov_agent.trn.log
diffing the instruction sequences...
saving to /work/ropeders/src/core-v-verif/cv32e40s/sim/uvmt/xrun_results/default/hello-world/0/isacov_logdiff
FAIL

And the log itself starts like this:

75c75
< c.lw x15, 72(x14)
---
> c.lw x15, x14, 0

It is not a perfect validation of isacov, but it is capable of highlighting some problems. (All the errors in the output from that hello-world run happens to be about RVC.)

[AyoubJalali]

@silabs-robin, think you for this, i think it will be very helpful for me. Answering your question , I was investigating some missing coverage (bins related to value type), and I review the code manually. I'm totally agree that if we replace the get_field_imm with get_data_imm, that not gonna help to reveal like these kind of error, but I have an idea to passed in the width argument the size of the value directly like $bits(value) for example, in this way we are going to avoid some error like this or a copy-paste error, or a incorrect count of the imm field in the spec.