core-v-verif
core-v-verif copied to clipboard
openhwgroup
ISACOV : No rs1 for c.mv & c.add instructions
Hello, @MikeOpenHWGroup @silabs-robin Always in isacov, I notice that there some cover group like, cp_rs1_toggle & cp_rs1_value in c.mv & c.add instructions, but these instructions don't use rs1, so it's better to remove these cover group, and also in c_has_rs1 typedef should be reviewed https://github.com/openhwgroup/core-v-verif/blob/383dbc81f2292a785c05bea8125d157f42aa8db5/lib/uvm_agents/uvma_isacov/uvma_isacov_tdefs.sv#L559 & https://github.com/openhwgroup/core-v-verif/blob/383dbc81f2292a785c05bea8125d157f42aa8db5/lib/uvm_agents/uvma_isacov/uvma_isacov_tdefs.sv#L561
I believe you are right @AyoubJalali. The two lines in uvma_isacov_tdefs.sv that you point out can be removed. Also, in uvma_isacov_cov_model.sv, the covergroup cg_cr should have the following removed:
cp_rs1_value: coverpoint instr.rs1_value_type {
ignore_bins POS_OFF = {POSITIVE} with (!rdrs1_is_signed);
ignore_bins NEG_OFF = {NEGATIVE} with (!rdrs1_is_signed);
ignore_bins NON_ZERO_OFF = {NON_ZERO} with (rdrs1_is_signed);
}
...and...
`ISACOV_CP_BITWISE(cp_rs1_toggle, instr.rs1_value, 1)
@silabs-robin, what do you think?
Hello folks.
Good catch Ayoub. That's sharp.
cp_rs1_toggle & cp_rs1_value in c.mv & c.add instructions, but these instructions don't use rs1
For c.mv you are right that it doesn't use rs1.
It merely has the rd/rs1 field in its format, but it only uses the rd part of it.
Therefore, I agree that C_MV can be removed from the c_has_rs1 list.
(Although, the name of that list could then be more accurately be reflected by a name like c_uses_rs1?)
For c.add I am not sure if I agree yet.
It seems to be using rs1?
The spec says C.ADD expands into add rd, rd, rs2, where rd is taken from the rd/rs1 field, so in effect it is add rs1, rs1, rs2 or add rd, rs1, rs2 (with rd==rs1).
Hence I (currently) think that 1) C_ADD belongs in the c_has_rs1 list, and 2) the cg_cr covergroup should keep the cp_rs1_value coverpoint.
To differentiate in the covergroup between those that do use the rs1 part of the rd/rs1 field, I think there are two options: 1) Create a sibling covergroup that is identical except the minor differences, or 2) use flags to enable and filter in/out the relevant parts.
I prefer option 2 as far as that is possible, so that there is one covergroup per instruction type/format.
@silabs-robin, I totally agree with you, so the solution is to use the c_has_rs1, because the c.mv & c.add share the same format, so we could remove c.mv from c_has_rs1 and keep c.add, and we use it as a condition to create the covergroup for rs1.
Sounds like we are in alignment. @AyoubJalali, can you create a pull-request with these fixes and improvements? Thanks!