Fix incomplete merge with multiple covergroups in a covergroupCoverage #19
Conversation
|
Hi @KasperHesse, thanks for reaching out with this report. I agree that the results you see aren't expected. The solution might require more discussion. I've been looking back over the UCIS spec, and looking at the UCIS XML reader code. My understanding is:
I would expect the UCIS Viewer to display the type name (eg uvm_test_top.env.master.coverage.cg_addr) instead of 'default'. Generally speaking, 'default' is shown when some optional name/identifier isn't present in the XML. Based on looking at your example, I would expect to see:
Does this look as you expect? ~Matthew |
|
Hi Matthew,
Obviously this would require that we don't have duplicate covergroup names anywhere in the testbench. If that were the case, it would also be necessary to identify the name of the class in which the covergroup is instantiated, as shown in brackets. An easy fix would be to replace the following code in pyucis/src/ucis/xml/xml_reader.py Lines 152 to 160 in 2bc9afd with cg_typescope = inst_scope.createCovergroup(
self.getAttr(instances[0], "name", "default").split(".")[-1],
None,
1,
UCIS_OTHER
)To get the name of the covergroup. See below. This correctly merges covergroups with the current merging-code, but could inadvertently merge duplicate covergroup names from unrelated coverage collectors. |
|
Hi Kasper, Thanks, |
|
Hi Matthew, |
|
Hi Kasper, |
|
Hi Matthew, Experiment 1Here, I'm using the following Python code to generate a coverage report. I'm defining two covergroups, and instantiating two of each. I generate some dummy data that is sampled before generating the XML coverage report. Python code to generate coverageimport vsc
import random
@vsc.covergroup
class first_covergroup(object):
def __init__(self, name):
self.options.name = name
# self.options.per_instance = True
self.with_sample(
rdwr = vsc.bit_t(1),
addr = vsc.bit_t(8),
data = vsc.bit_t(8)
)
self.cp_rdwr = vsc.coverpoint(self.rdwr, bins={
"RD" : vsc.bin(0),
"WR" : vsc.bin(1)
})
self.cp_addr = vsc.coverpoint(self.addr, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_data = vsc.coverpoint(self.data, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_cross = vsc.cross(
[self.cp_rdwr, self.cp_addr, self.cp_data]
)
@vsc.covergroup
class second_covergroup(object):
def __init__(self, name):
self.options.name = name
# self.options.per_instance = True
self.with_sample(
addr = vsc.bit_t(8),
data = vsc.bit_t(8)
)
self.cp_addr = vsc.coverpoint(self.addr, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_data = vsc.coverpoint(self.data, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
if __name__ == "__main__":
# Create two of each covergroup, generate 10 random samples for each covergroup
fcg_1 = first_covergroup("first_covergroup_1")
fcg_2 = first_covergroup("first_covergroup_2")
scg_1 = second_covergroup("second_covergroup_1")
scg_2 = second_covergroup("second_covergroup_2")
fcg_1_rdwr = [random.randint(0,1) for _ in range(10)]
fcg_1_addr = [random.randint(0,255) for _ in range(10)]
fcg_1_data = [random.randint(0,255) for _ in range(10)]
fcg_2_rdwr = [random.randint(0,1) for _ in range(10)]
fcg_2_addr = [random.randint(0,255) for _ in range(10)]
fcg_2_data = [random.randint(0,255) for _ in range(10)]
scg_1_addr = [random.randint(0,255) for _ in range(10)]
scg_1_data = [random.randint(0,255) for _ in range(10)]
scg_2_addr = [random.randint(0,255) for _ in range(10)]
scg_2_data = [random.randint(0,255) for _ in range(10)]
for i in range(10):
fcg_1.sample(fcg_1_rdwr[i], fcg_1_addr[i], fcg_1_data[i])
fcg_2.sample(fcg_2_rdwr[i], fcg_2_addr[i], fcg_2_data[i])
scg_1.sample(scg_1_addr[i], scg_1_data[i])
scg_2.sample(scg_2_addr[i], scg_2_data[i])
vsc.write_coverage_db(f"cov.xml")The coverage is attached as I've used PyUCIS to merge the coverage DB with itself (attached as Merging the coverage report using my "fix" generates the following (attached as cov.txt As you can see, with my modification, the coverage is correctly merged. However, the TYPE label is still "default", likely because a hierarchical naming scheme isn't used. Since we're using PyUVM and hierarchical naming in our project, that also warrants attention, leading us to: Experiment 2Now, a hierarchical system of coverage collectors is used. The code is as follows Python code for experiment 2The definitions of the covergroups haven't been changed.import vsc
import random
@vsc.covergroup
class first_covergroup(object):
def __init__(self, name):
self.options.name = name
# self.options.per_instance = True
self.with_sample(
rdwr = vsc.bit_t(1),
addr = vsc.bit_t(8),
data = vsc.bit_t(8)
)
self.cp_rdwr = vsc.coverpoint(self.rdwr, bins={
"RD" : vsc.bin(0),
"WR" : vsc.bin(1)
})
self.cp_addr = vsc.coverpoint(self.addr, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_data = vsc.coverpoint(self.data, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_cross = vsc.cross(
[self.cp_rdwr, self.cp_addr, self.cp_data]
)
@vsc.covergroup
class second_covergroup(object):
def __init__(self, name):
self.options.name = name
# self.options.per_instance = True
self.with_sample(
addr = vsc.bit_t(8),
data = vsc.bit_t(8)
)
self.cp_addr = vsc.coverpoint(self.addr, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
self.cp_data = vsc.coverpoint(self.data, bins={
"low" : vsc.bin([0, 84]),
"med" : vsc.bin([85, 169]),
"high" : vsc.bin([170, 255])
})
class cov_collector():
def __init__(self, name):
self.name = name
self.first = first_covergroup(f"{name}.first_covergroup")
self.second = second_covergroup(f"{name}.second_covergroup")
def do_cov(self):
first_rdwr = [random.randint(0,1) for _ in range(10)]
first_addr = [random.randint(0,255) for _ in range(10)]
first_data = [random.randint(0,255) for _ in range(10)]
second_addr = [random.randint(0,255) for _ in range(10)]
second_data = [random.randint(0,255) for _ in range(10)]
for i in range(0,10):
self.first.sample(first_rdwr[i], first_addr[i], first_data[i])
self.second.sample(second_addr[i], second_data[i])
if __name__ == "__main__":
cov_mst = cov_collector("mst")
cov_slv = cov_collector("slv")
cov_mst.do_cov()
cov_slv.do_cov()
vsc.write_coverage_db(f"cov_hier.xml")Generated coverage Merged coverage, original code Merged coverage, my modification TakeawayI believe my proposed modification to If "." is not used as the separator, the TYPE labels will be somewhat misleading, but the merge will still happen correctly (pictured below with "," as separator. While the TYPE label is "mst,first_covergroup", the correctly merged and placed instances of "first_covergroup" under the same TYPE label) |
Previous merge behaviour did not fully merge coverage files when multiple covergroups existed under a single
<covergroupCoverage>.Consider the following XML file generated when collecting coverage
cov.xml
The XML file contains 2x
<covergroupCoverage>, each of which contains two<cgInstance>. When viewing withpyucis_viewer, thecovergroupCoveragesections are labeled "default" since multiple covergroups exist beneath them.When merging this coverage file with another coverage file from a similar run, the coverage merger only identifies one covergroup name (default), and thus doesn't include all nested covergroups. In the following, see that only the last covergroup from each section is included (
cg_delays)This PR fixes this by including all covergroups under a heading, not just the final one discovered (the one coming last in an alphabetical sorting). In the end, all covergroups are placed under a single "default" heading.
An alternative to this PR would be to modify the underlying database structure such that each
covergroupCoverageis only associated with onecgInstance, but this might break spec conformance. Alternatively, auserAttrcould be added to eachcovergroupCoverageto name thecovergroupCoveragesince adding anameentry directly in the XML field does not conform to spec.