Q: Disabled type coverage

[eyck]

In commit 207076a the type coverage calculation has been completely disabled. Why's that?

[mballance]

Type coverage is expected to reflect the merge of instance coverage. For example, given the instance-coverage data below:

pyucis/ve/unit/test_merge.py

Lines 58 to 81 in 62bf38c

	coverage:
	covergroups:
	- name: cvg
	instances:
	- name: inst1
	coverpoints:
	- name: cp1
	bins:
	- name: b0
	count: 0
	- name: b1
	count: 1
	- name: inst2
	coverpoints:
	- name: cp1
	bins:
	- name: b0
	count: 1
	- name: b1
	count: 0

The expected type coverage would be 100%.
For reporting purposes, coverage achieved for an covergroup (type or instance) is the weighted sum of coverage achieved by its coverpoints and crosses. The commented code was adding instance coverage to this weighted sum, which is incorrect. In the example above, adding in the weighted sum of instance coverage resulted in type coverage being reported as 66.67% ((100+50+50)/3) which is clearly not correct.

[eyck]

I somehow disagree for several reasons. For one reason using the attached UCIS XML
TGC_C_AHB.ucis.xml.gzI get 0% type coverage if the cover group only contains instances.
For another reason I do not follow your arguments: if there are 3 cover points (underneath the top covergroup) where 2 of them are only covered at 1/2 the weighted sum is only 2/3. Your argumentation drops the covergroups being part of the top covergroup (unfortunately in your example they all are called cg1)

[eyck]

I did some research. I guess the disagreement comes from union merge vs. non-union merge (as described here: https://tenthousandfailures.com/blog/2014/1/5/merging-system-verilog-covergroups-with-examples).
I implemented a check for this here: cefb748
@mballance if you think this is a viable solution I would create a PR

[mballance]

Hi @eyck, thanks for sharing the link to Eldon's article. Good reminder of the differences between the two key ways coverage can be merged/reported. I think we have two areas of misalignment. One is on where data needs to be 'adjusted' and the other is how to appropriately handle these two cases.
I'm starting from a position that favors the 'union merge' semantics -- it's just what I'm most familiar with. So, when I see a coverage format (FC4SC captured as XML) that only represents instance coverage, my assumption is that the reader for that coverage capture will reconstruct the type coverage data (if type coverage is relevant). PyUCIS expects this reconstruction to happen before any API client attempts to access the data (ie it must happen in XmlReader).
I'm a bit confused by the screenshots of non-union merge in the article. Despite specifying 'no instance coverage', Questa still shows instance data. But, leaving that aside...
I think the following makes sense as a steps forward:

• There may be some work required to properly read/write coverage options via the PyUCIS API. I'll investigate on this.
• If there are cases where FC4SC users expect to view 'union merge' semantics, let's have the XML reader add in type-coverage data. I assume this could be done only if the relevant options are set
• We can change the reporter to report type coverage as a weighted average of instance coverage when the relevant options are set to select non-union merge. It think this would be restoring the code removed in 207076a.

[eyck]

I'm a bit confused by the screenshots of non-union merge in the article. Despite specifying 'no instance coverage', Questa still shows instance data. But, leaving that aside...

This is exactly the commented part of 207076a: the weighted sum of the instance coverages. Therfore only some percentage and no bins or crosses.

---

• We can change the reporter to report type coverage as a weighted average of instance coverage when the relevant options are set to select non-union merge. It think this would be restoring the code removed in 207076a.

Actually I would assume: if there is some coverage data in the covergroup than some preprocessing has been done (e.g. a union-merge). If this is not the case than the reporter should fall-back to non-union merge coverage reporting. This is how I implemented it atm in our fork.

[mballance]

Okay, I'm still working to build my understanding of how UCIS represents these cases. Based on that understanding, it should be clear whether changes are needed to the XML reader.
6.4.3.12 Other Information Models for Covergroups

• per_instance=0 allows, but doesn't require, per-instance data to be omitted. When per_instance=0 and merge_instances=1, type-coverage data must be present. The UCIS_REAL_CVG_INST_AVERAGE property should contain the weighted-average of all type coverpoints.
• When per_instances=0 and merge_instances=0, both type data and instance data should be omitted. However, the UCIS_REAL_CVG_INST_AVERAGE property must contain the weighted-average of all type coverpoints

I think this means the following in terms of the coverage report:

• The way type coverage is reported must change based on the setting of per_instance and merge_instances. Specifically, if per_instance=0 and merge_instances=0, type coverage should report the value of UCIS_REAL_CVG_INST_AVERAGE instead of attempting to compute coverage from bin data
• If per_instances=0 and merge_instances=1, the coverage report must report type coverage based on data in the coverage bins. It could choose to omit reporting instance coverage even if the instance data is present.

I think this means the following in terms of the XML data format:

• XML writers (eg FC4SC) must properly record per_instance and merge_instances options
• The XML reader must read the per_instance and merge_instances properties and write them via the UCIS API
• The XML reader must reconstruct type-coverage data when merge_instances=1
• The XML reader must compute weighted-average coverage for covergroup types and record it in the UCIS_REAL_CVG_INST_AVERAGE property

Thoughts?

[eyck]

I agree with the first and the second item. But I'm not sure if the XML reader shall reconstruct or compute the coverage. This contradicts the principle of separation of conserns. So either this is the job of the report generator or the database itself, not the XML reader.