core-v-verif icon indicating copy to clipboard operation
core-v-verif copied to clipboard

ISACOV: Sequential sampling incorrect

Open silabs-robin opened this issue 3 years ago • 9 comments

Type

  • Functionally incorrect behavior

Steps to Reproduce

Run any test/regression and look at rev32_seq_cg.

Additional context

It seems like the sampling of sequences of instructions is non-functional.

When I look at coverage metrics for a "full" regression, it seems only "A->A" transitions are covered (e.g. "ADDI->ADDI"). (See screenshot below.) This indicates that something about the coverage definition or its sampling is not working as intended. @AyoubJalali are you seeing the same results as I do?

I don't know what is needed to get this working, but the principle is simple so it could potentially be easy.

image


ps. For "instruction group" coverage, it seems to be working. Except that we need to exclude "UNKNOWN_GROUP".

image

silabs-robin avatar Oct 27 '22 10:10 silabs-robin

@AyoubJalali are you seeing the same results as I do?

Yes, exactly same result, i didn't notice that because for me it the last cg to look for now (for me).

Except that we need to exclude "UNKNOWN_GROUP".

Yes because no sample if instr = UNKNOWN

AyoubJalali avatar Oct 27 '22 11:10 AyoubJalali

@silabs-robin look at this https://github.com/openhwgroup/core-v-verif/blob/05ba4eb0cf6296bf35f487fbe2ab01e532d0fe26/lib/uvm_agents/uvma_isacov/cov/uvma_isacov_cov_model.sv#L1399

isn't we should create cover group for instr_prev.name instead of instr.name ?? like this

cp_instr_prev_x2: coverpoint(instr_prev.name) iff (instr_prev != null) { `ISACOV_IGN_BINS ignore_bins IGN_X2_OFF = {[0:$]} iff (!seq_instr_x2_enabled); }

AyoubJalali avatar Oct 27 '22 11:10 AyoubJalali

isn't we should create cover group for instr_prev.name instead of instr.name ?? like this

That seems to be it, yeah :)

silabs-robin avatar Oct 27 '22 11:10 silabs-robin

Yes it works for me

AyoubJalali avatar Oct 27 '22 12:10 AyoubJalali

Nice. Looking forward to re-measuring our coverage with all of the improvements you have made.

I will close this issue after a related PR gets merged.

silabs-robin avatar Oct 27 '22 13:10 silabs-robin

Best luck to get 100% :)

AyoubJalali avatar Oct 27 '22 13:10 AyoubJalali

I will close this issue after a related PR gets merged.

@silabs-robin can you provide the actual PR number here? Thanks.

MikeOpenHWGroup avatar Oct 27 '22 15:10 MikeOpenHWGroup

I will close this issue after a related PR gets merged.

@silabs-robin can you provide the actual PR number here? Thanks.

There is no PR number yet AFAIK. But, @AyoubJalali, you have a fix for the problem, so you might soon have a PR up? If you @ me in that PR I can link to it here, or if you reference this issue in the PR, then Mike's request will be fulfilled.

silabs-robin avatar Oct 27 '22 15:10 silabs-robin

No problem for me, I'll reference this issue in the PR

AyoubJalali avatar Oct 27 '22 15:10 AyoubJalali