Isacov agent: coverage model

[AyoubJalali]

Hi @MikeOpenHWGroup ,I have been working on reusing the ISACOV agent for CVA6, and i notice in the cover group declaration related to C extension, specifically rv32c_jal_cg & rv32c_j_cg you assign rd_is_signed to imm_is_signed , i didn't get this because i think we should assign imm_is_signed the c_imm_is_signed, because the C_J & C_JAL instructions don't use a destination register, I talk about https://github.com/openhwgroup/core-v-verif/blob/05ba4eb0cf6296bf35f487fbe2ab01e532d0fe26/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L2045 and https://github.com/openhwgroup/core-v-verif/blob/05ba4eb0cf6296bf35f487fbe2ab01e532d0fe26/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L2047 in rv32c_j_cg & rv32c_jal_cg.

[MikeOpenHWGroup]

Thanks @AyoubJalali. I think you are right.

The definition of rd_is_signed does not include either C_J or C_JAL. Rather, these are defined in c_imm_is_signed.

@silabs-halfdan, @silabs-mateilga and @silabs-robin do any of you have an opinion or insight on this?

[silabs-robin]

Hi @AyoubJalali. I think you are right indeed. This looks like a copy-paste bug, potentially. C_J and C_JAL does not use "rd", and the "imm_is_signed" should be connected like you propose. (Seems like with the current setup it even gets a 0 where 1 should be correct?)

---

Additional thoughts:

Maybe those "default:" values are dangerous? I don't know the answer right now, but we could consired removing the defaults.

The last time I worked on isacov, I found shortcomings in the coverage of the C extension and I only had time to fix some of it.

The other issue I know about (from the "top of my head") is toggle coverage. I remember there was something wrong with the macros like ISACOV_CP_BITWISE_11_1, but I don't remember if I fixed it and if I fixed all of it.

I'm just mentioning these things because I want to get some overview of the state of things and get some attention towards fixing potential flaws.

[AyoubJalali]

Hello @silabs-robin , thanks for your feedback, I'm with the idea of removing default values, because could be assigning wrong value because of a copy-paste bug, and I would like to know more about these bugs in the ISACOV, because currently i'm reusing it for CVA6, and i found some, but the main one is that the ISACOV developed for ILEN=32 & XLEN=32, but the CVA6 has a different general purpose register length (XLEN=64), so i change some function and variables, i'll appreciate if you let me know if there is some other bugs in isacov.

[silabs-robin]

I'm with the idea of removing default values

Removing defaults would be nice if it could catch errors such as the one we just saw.

However, as I look at it more closely now, all of those arrays use instr_name_t as an index type, so if we drop defaults then 1) we would have to tediously specify a very long list of other instructions, and 2) such a long list would have (at least) equal likelyhood of producing a similar type of mistake. If the index type was something more specific than the current instr_name_t then it could be some worth in that, but changing that again could potentially be more work than what it is worth.

I don't immediately see a cleaner way to do this, but if you have any ideas then I am much in favor of changing this code into something more cautious and robust.

I would like to know more about these bugs in the ISACOV

Certainly. I tried to backtrack and dig up what I know.

As I said, "toggle coverage" doesn't seem right for C extension, as indicated by this todo-comment: https://github.com/openhwgroup/core-v-verif/blob/master/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L1107 You can see on the datatypes in uvma_isacov_instr_c and in the immediate-extracting function etc that the macros used for toggle coverage seems off (you'd probably see this in coverage metrics too anyway).

I also found an old issue I created about this at https://github.com/openhwgroup/core-v-verif/issues/1076. Some of it is just "nice-to-haves" but it seems the only things marked as "bugs" are about toggle coverage. isacov_logdiff might help in uncovering issues; the idea of it is to reproduce a log similar to the rvfi log to compare what isacov has seen vs what the core's tracer has seen.

[AyoubJalali]

@silabs-robin I notice this problem with the toggle coverage with C extension because of the filed length in the compressed instruction. Thank you that was helpful, and if i had some idea i'll share with you.

[MikeOpenHWGroup]

Great discussion all (and a fabulous example of how open-source code produces quality results).

I am not sure if we have a resolution here. At a minimum we need a pull-request to resolve the original issue flagged by @AyoubJalali. Do we need another one, or is #1076 sufficient for that?

[AyoubJalali]

@MikeOpenHWGroup @silabs-robin ,if i'm not wrong, there is more of this copy-paste bug, please look at : https://github.com/openhwgroup/core-v-verif/blob/05ba4eb0cf6296bf35f487fbe2ab01e532d0fe26/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L1756

https://github.com/openhwgroup/core-v-verif/blob/05ba4eb0cf6296bf35f487fbe2ab01e532d0fe26/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L1687

it's the same value for ADDI, but we should correct rs1 with rd for ADDI instruction, but for JALR in I extension we have different values , and there is more like this.

[MikeOpenHWGroup]

At a minimum we need a pull-request to resolve the original issue flagged by @AyoubJalali

Hi @AyoubJalali, please create a pull-request to resolve both of these issues. Thanks!

[AyoubJalali]

@MikeOpenHWGroup on the master branch ?

[MikeOpenHWGroup]

@MikeOpenHWGroup on the master branch ?

Great question, I should have specified that. Because you are doing this work for the CVA6, please submit the PR to the cva6/dev branch. All dev branches are periodically merged up to the master branch and then down across to other dev branches as required. This will allow the cv32e40 cores to decide for themselves when and whether they need this update.

[AyoubJalali]

@MikeOpenHWGroup here is the PR https://github.com/openhwgroup/core-v-verif/pull/1405

[silabs-robin]

if i'm not wrong, there is more of this copy-paste bug

That might be the case. But, if you are fixing these, please keep in mind to double check vs the riscv spec whether some of these could be intentional. I don't know for sure, but I suspect that (because of irregularities between the instruction formats) that some fields might have been "cross connected" if their bit position is at the same place. (Thought I would consider it bad practice if that has been the case.)