Feature Request: Functional coverage from OSVVM

Blebowski commented 1 week ago

Multiple resources refer that OSVVM supports functional coverage, e.g.: DOULOS OSVVM page

It would be nice if NVC functional coverage could report this functional coverage too (as part of functional coverage). OSVVM allows generating HTML report from the TB itself, but it may be more convenient for user to have this integrated.

Functional bins for OSVVM functional coverage objects can be added at run-time by calls to AddBins. So coverage bins are dynamic objects allocated at run-time. OSVVM then exposes vendor interface to access this information (VendorCovAPIPkg.vhd). Currently it only implements ALDEC interface.

NVC implementation of the interface would need to provide implementation of functions from VendorCovAPIPkg.vhd.

I have not yet experimented with this, but I would like to get to it at some point.

tmeissner commented 1 week ago

That would also interesting for me. Some years ago I had an own OSVVM branch implementing XML exports which were further processed with Python scripts.

Blebowski commented 1 week ago

A similar feature is available in UVVM:

UVVM Functional coverage

except that it does not have simulator API.

UVVM commented 1 week ago

We would like to add this functionality to UVVM as well.

Blebowski commented 6 days ago

Hi @nickg ,

I am trying to play with this to see how far I get.

My approach is:

Modified OSVVM to provide Vendor COV API for NVC: OSVVM branch and provided run-time calls for these.
During lower phase, I emit coverage scope and single item for each OSVVM covPType or CoverageIDType signal or variable declaration.
At run-time, OSVVM calls VendorCovPointCreate and requests a "simulator specific handle" for the coverage object. All later calls for the coverage object are identified by this handle. I would like to return pointer to the coverage scope item emited in Step 2.
During each call to VendorCovBinAdd, the run-time code gets the handle, and adds next coverage item into the coverage scope. NVC re-allocates run-time counters holding coverage data (to account for new item being added at run-time). This-way coverage counting in OSVVM internals and NVC coverage counters are synced. Since run-time coverage counters are identifier by tag (offset in run-time data, rather that direct pointer), re-allocating the coverage counters memory does not mess-up the whole thing.
During each call to VendorCovBinInc, the run-time code is simply able to locate cov_item based on index, and therefore access run-time data that correspond to this coverage item.

I am having trouble with step 3 though. In the call to VendorCovPointCreate, I need to locate the underlying signal or variable that lead to the call. I parse jit_stack_trace result and get first user-level entry (lowest call scope that is not in OSVVM itself). Since call stack has loc, I search the tree for matching loc_t and valid object type. Thisway it is fairly easy to find tree_t for variable or signal declaration of covPType or CoverageIDType that lead to the call.

Here I am stuck since from the tree_t osvvm_obj I can't determine its coverage scope. Iterating run-time scopes upwards and searching for identical coverage scope is not possible since these scopes are different (coverage scope creates entry for each nested statement, process, etc...).

I though of following of solutions:

Adding object_t to cover_item_t, and search coverage scopes and compare tree_to_object(ossvm_obj) with per coverage item tree_t . But cover items are emmited during elab phase. Is it correct to assume that this pointer will not be valid at run-time if -e and -r will be two separate invocations of NVC ? So the serialization and deserialization of tree_t destroys the reference ?
Adding cover_item->hier to the tree_t (e.g. as IDENT2), during lower phase. But this fatals since I get "Attempt to write to frozen arena".
Somehow emmiting a run-time code that would contain the coverage scope identifier, and that would hook to a run-time call of another function. Such function would then do the mapping since it would get pointer to the tree_t and coverage_scope_t path. But the tree_t persistence would still be problem.

All of these seems too hacky, so I though I would rather ask. Do you have any hint on how to approach this ?

My implementation is here: NVC branch

nickg commented 5 days ago

The simulator shouldn't have any knowledge about types and subprograms in third-party libraries. The right way to go about it is to provide a library that users can call into to create custom coverage scopes and items, similar to what Aldec has. There's nothing preventing the coverage model being extended at runtime so the code generation step doesn't need to know anything about this.

I added a nvc.cover_pkg package in ef6e673dcb which adds a basic VHDL interface to the coverage system. This should be sufficient to implement the OSVVM API on top of, perhaps with a few minor additions.

Blebowski commented 5 days ago

Thanks, I will give it a try.

Blebowski commented 4 days ago

I gave it a shot at integrating the cover_pkg. It is a WIP with more created issues than resolved ones.

The OSVVM Vendor API PKG returns VendorCovHandleType handle (which is integer), while cover_pkg returns t_scope_handle, a pointer to t_opaque_scope. Since pointer will be 64 bit, but pre-VHDL2019 integer is 32 bit, I guess it can't be directly typecasted?

I did not want to change the base-type of VendorCovHandleType, so I tried to implement dynamic array holding the pointers to t_scope_handle. I wrapped it in protected type to get atomicity. Since it handles access type, I needed to compile OSVVM with --relaxed. The code is here: OSVVM NVC COV API There is little handling logic around, but it is more-less OK.

I have slightly more trouble with the coverage item / scope emission. The current approach searches for run-time scope from where create_cover_scope was called. This will have trouble e.g. in scenario:

my_block : block
    begin
        cp1 : process
            variable CP : CoverageIDType;
        begin
            CP := NewID("DUMMY") ;
            AddBins(CP, "Bin 1 to 2", GenBin(1,2,1));
            AddBins(CP, "Bin 3 to 4", GenBin(3,4,1));
            -- Wait for some time and then do the sampling here
            wait;
        end process cp1;

        cp2 : process
            variable CP : CoverageIDType;
        begin
            CP := NewID("DUMMY") ;
            AddBins(CP, "Bin 100 to 200", GenBin(100,200,1));
            AddBins(CP, "Bin 300 to 400", GenBin(300,400,1));
            -- Wait for some time and then do the sampling here
            wait;
        end process cp2;

The current approach will determine calling rt_scope_t as: <WHATEVER_HIER>.MY_BLOCK. If user passes empty or equal cover point name when constructing both bins, both added scopes will be something like:

<WHATEVER_HIER>.MY_BLOCK.DUMMY

so these will alias during merging and merged COVDBs will be corrupted. Also, it is non-intuitive that the coverage data will be located in hierarchy where the AddBins is first called, rather than where CoverageIDType signal / variable is defined. If a shared variables with cover points were defined in a package so that user can e.g. initialize and sample them in one entity, but do reporting in another entity, then these items would be reported under the entity where the AddBins is called. This is confusing and frankly, I don't like it. All the rest of the NVC code-coverage is based on hierarchy where the given item is declared (e.g. toggle or FSM coverage).

Much better would be having something that locates the coverage hierarchy of declaration of the CoverageIDType signal / variable, and emits such scope. The example above would create coverage scopes like so:

<WHATEVER_HIER>.MY_BLOCK.CP1.CP
<WHATEVER_HIER>.MY_BLOCK.CP2.CP

This would prevent scope naming conflicts if user would do something messy. OSVVM itself does not check for name duplicities.

I tried to do a little bit of hacking around and parsing the call-stack and creating unique cover scope name:

by appending all nested user function calls
by appending identifier of tree_t declaration of the OSVVM object. This can be found in _nvc_get_osvvm_object_suffix in the: NVC OSSVM branch

For the case of: OSVVM_TEST

I get emmited coverage scopes like so:

WORK
  OSVVM_TEST_WRAPPER : instance
    MY_INSTANCE : instance
      STIM_GEN
      MY_BLOCK
        INITCOVERAGE
        INITCOVERAGE.COV1.DUMMY
          0: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.COV1.DUMMY.BIN_3_TO_5 osvvm_test.vhd:58 => 0
          1: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.COV1.DUMMY.BIN_3_TO_5 osvvm_test.vhd:58 => 0
          2: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.COV1.DUMMY.BIN_11_TO_20 osvvm_test.vhd:58 => 0
        INITCOVERAGE.COV2.DUMMY
          3: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.COV2.DUMMY.BIN_10_TO_12 osvvm_test.vhd:58 => 0
        INITCOVERAGE.CP_B
          4: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.CP_B.BIN_3_TO_5 osvvm_test.vhd:58 => 0
          6: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.CP_B.BIN_6_TO_8 osvvm_test.vhd:58 => 0
        INITCOVERAGE.CP_A_ADD_FIRST_BINS.CP_A
          5: cover point WORK.OSVVM_TEST_WRAPPER.MY_INSTANCE.MY_BLOCK.INITCOVERAGE.CP_A_ADD_FIRST_BINS.CP_A.BIN_3_TO_5 osvvm_test.vhd:58 => 0
      SAMPLE
      COVERREPORT

which sort-of avoids the conflict by appending process name, searched OSVVM declaration name, and right now also the user entered name into NewID. The user entered scope name will go away from the hierarchy and will be remembered somewhere separately (e.g. in func_name attribute). The same for bin_name.

But the issue still remains. E.g. the last cover point CP_A is defined in my_pkg package, but it will be reported in the my_block.

Due to this I originally opted for emmiting the cover scope at the time of CoverageIDType definition in the lower phase. Then later on in run-time during the call to VendorCovPointCreate the handle to such cover scope would be looked-up and linked to OSVVM handle.

I wouldn't like to create mess out of it, so I rather ask what approach would you choose ?

nickg commented 4 days ago

I did not want to change the base-type of VendorCovHandleType, so I tried to implement dynamic array holding the pointers to t_scope_handle.

Why not? Maybe check with @JimLewis but it seems to me that the whole point of having the Vendor*Type types and a separate packages per vendor is that the types don't have to be the same. You should be able to do

subtype VendorCovHandleType is t_scope_handle.

The current approach will determine calling rt_scope_t as: <WHATEVER_HIER>.MY_BLOCK. If user passes empty or equal cover point name when constructing both bins, both added scopes will be something like:
<WHATEVER_HIER>.MY_BLOCK.DUMMY
so these will alias during merging and merged COVDBs will be corrupted.

The problem here is just that the names lack validation and de-duplication. That's easy to fix, see d5e3844.

Also, it is non-intuitive that the coverage data will be located in hierarchy where the AddBins is first called, rather than where CoverageIDType signal / variable is defined. If a shared variables with cover points were defined in a package so that user can e.g. initialize and sample them in one entity, but do reporting in another entity, then these items would be reported under the entity where the AddBins is called. This is confusing and frankly, I don't like it. All the rest of the NVC code-coverage is based on hierarchy where the given item is declared (e.g. toggle or FSM coverage).

It's not non-intuitive at all. It seems to me the most logical way to do it and it's exactly how the Aldec interface behaves. See https://www.edaplayground.com/x/n3Af and the XML generated from it xml.zip. The user-defined cover-groups are all attached to the instance rather than the process/procedure that created them.

What is non-intuitive is attempting to do some magic stack parsing that somehow has knowledge of OSVVM's as-of-today API and data-types and tries to guess where the user wanted it created. I don't see how that could ever work with something like:

impure function my_wrapper (s : string) is
    variable id : CoverageIDType;      -- Is it in the scope under my_wrapper?
begin
    id := NewID(s);
    return s;
end function;

...

process is
    variable id1, id2 : CoverageIDType;
begin
    id1 := my_wrapper("foo");    -- ... or in the process scope?
    id2 := id1;                  -- Does this create a second copy?
    wait;
end process;

The handle is just a value, we shouldn't attach any significance to the place where it's stored.

This feature should be provided as a generic API that can be used by OSVVM, UVVM, or any random user. Anything that ties it to a particular framework is a mistake.

JimLewis commented 4 days ago

Being the second simulator to go through this activity, I would expect you to find some rough edges that need updated.

OSVVM CoveragePkg should not know anything about the types defined in VendorCovApiPkg.vhd such as VendorCovHandleType.

That said, currently, CoveragePkg initializes the value as follows:

CovStructPtr(Index).VendorCovHandle    := 0 ;

This is gross in that it assumes that the type is some sort of integer type. This should be replaced by a constant defined in the package and updated as:

CovStructPtr(Index).VendorCovHandle    := VENDOR_COV_HANDLE_INIT ;

WRT CoveragePkg types, CovPType is our old stuff. CoverageIdType is the new stuff. Please focus on CoverageIdType.

I would not feel bad if you did not support functional coverage integration for CovPType - or the names came out strange for it. OSVVM does soft deprecation of features, so CovPType still exists and is usable, but is not documented any more. Other approaches would have deprecated and removed it already.

I think if you report the path to the functional coverage object, reporting the location of the declaration of the object is ok. Honestly I have not looked at what SystemVerilog tools do, but I expect that they print the name of the coverage model and not the path to its declaration. So an option to not print just the functional coverage object would be nice.

Another direction, that may incur less overhead at run time, would be to parse the coverage yaml files that OSVVM creates when you call EndOfTestReports.

Is there a native representation for functional coverage (other than the Accellera one as it is a square peg trying to fit in a polyhedron shape - sometimes it works great - others not so much)? OSVVM may be able to produce a different native representation as an alternative to the yaml files.

Currently OSVVM only tracks the name of the coverage object and does not keep the name of where it is defined (as that is challenging in VHDL).

nickg commented 4 days ago

Is there a native representation for functional coverage (other than the Accellera one as it is a square peg trying to fit in a polyhedron shape - sometimes it works great - others not so much)?

We have #933 for UCIS export. I've worked on it a bit but it's hard to find tools that actually support it: Questa implements the C API but not XML export, RivieraPRO has XML export but the format is totally different to the Accellera spec. Do you know of anyone actually using this?

JimLewis commented 3 days ago

I commented in #933, but I also found that neither imported XML as defined by the UCIS standard.

As a result, OSVVM creates a YAML file and then converts it to HTML. You can find some of our HTML reports here: https://osvvm.github.io/Overview/Osvvm3Reports.html Follow the links in the table as the reports there are the newer ones.

nickg commented 3 days ago

This is gross in that it assumes that the type is some sort of integer type. This should be replaced by a constant defined in the package and updated as:
CovStructPtr(Index).VendorCovHandle    := VENDOR_COV_HANDLE_INIT ;

In NVC's case the handle is an access type so we can't use a constant. How about adding a procedure that does this? E.g.

procedure VendorCovHandleDeallocate( obj: inout VendorCovHandleType );

Which would set obj to zero or null as appropriate. Maybe some simulators require the handle to be explicitly deallocated which this could also take care of.

Blebowski commented 3 days ago

Why not? Maybe check with @JimLewis but it seems to me that the whole point of having the Vendor*Type types and a separate packages per vendor is that the types don't have to be the same. You should be able to do

I ran into the issue @JimLewis mentioned (it needed to be integer type). I changed it in the CoveragePkg locally, but that would make it incompatible with Aldec. Isn't there some way how VHDL can "typecast" access type to int ?

It's not non-intuitive at all. It seems to me the most logical way to do it and it's exactly how the Aldec interface behaves. See https://www.edaplayground.com/x/n3Af and the XML generated from it xml.zip. The user-defined cover-groups are all attached to the instance rather than the process/procedure that created them.

I agree attaching to instance is OK as long as the deduplication / sanitization is taken care of in the NVCs coverage API. In the end it does not matter in which scope it lives as long:

It is documented somewhere
It does not have corner-cases that "uneducated user" can run into and break the whole thing.

The scope where something is created by f-call vs. scope where something is defined probably only hits on my HW level view of the world where objects (signals or variables) reside at firm place in hierarchy that defines them, rather than viewing them as dynamic objects, and seeing the variable definition only as pointer.

I assumed the cover point would be "attached" to its definition, which is probably wrong. The example you have shown will mess it up. If someone will create multiple cover points and assign them to a single handle, this would also completely mess-up the search for the underlying signal / variable definition.

What is non-intuitive is attempting to do some magic stack parsing that somehow has knowledge of OSVVM's as-of-today API and data-types and tries to guess where the user wanted it created. I don't see how that could ever work with something like:

You are right, it is magic. And it seemed too dirty to me, so I went on rather asking. Viewing the cover point from the HW perspective, rather than as dynamic object is the pitfall on my side here.

WRT CoveragePkg types, CovPType is our old stuff. CoverageIdType is the new stuff. Please focus on CoverageIdType.

I would not feel bad if you did not support functional coverage integration for CovPType - or the names came out strange for it. OSVVM does soft deprecation of features, so CovPType still exists and is usable, but is not documented any more. Other approaches would have deprecated and removed it already.

I would like to support both, since if you google, you will run into Doulos tutorials that use the legacy CovPType. I think with the way @nickg proposed to handle it, this should not be a problem. Both types call into VendorCovPointCreate. Call on CovPType will not pass any name, but there could be some simple heuristics that will create a new name (e.g. a number of scopes in the parent scope, and adding something like: <INST_WHERE_CREATED>.__NVC_UNNAMED_COVER_POINT_<XYZ>

Attaching an incrementing number will avoid duplicates.

Is there a native representation for functional coverage (other than the Accellera one as it is a square peg trying to fit in a polyhedron shape - sometimes it works great - others not so much)? OSVVM may be able to produce a different native representation as an alternative to the yaml files.

There is also the binary format, but it is not a good idea to try to write-out that one from OSVVM.

We have https://github.com/nickg/nvc/issues/933 for UCIS export. I've worked on it a bit but it's hard to find tools that actually support it: Questa implements the C API but not XML export, RivieraPRO has XML export but the format is totally different to the Accellera spec. Do you know of anyone actually using this?

At the time I opened that issue, in the company where I work we started to integrate: PyUCIS

in a system-level verification environment that verifies:

FPGA with CPU running production FW againts
Python model where python model provides reference functionality. We wanted to collect functional coverage on "what combinations of chip API are tested".

We exported the UCIS XML and managed to convert it into VCS covdb (to view the results), but we needed some hacking since VCS was not able to do it "out of the box".

Currently OSVVM only tracks the name of the coverage object and does not keep the name of where it is defined (as that is challenging in VHDL).

@JimLewis but it is possible to create duplicate names correct ?

JimLewis commented 3 days ago

I ran into the issue @JimLewis mentioned (it needed to be integer type). I changed it in the CoveragePkg locally, but that would make it incompatible with Aldec. Isn't there some way how VHDL can "typecast" access type to int ?

I will look at adding the procedure that @nickg suggested (vs. the constant I wanted but is not legal). This should be possible.

@JimLewis but it is possible to create duplicate names correct ?

Yes. That would be erroneous though - meaning user error.

With the singleton data structure, OSVVM could check for this, but currently does not. Many cases should be distinguishable by use the coverage name plus the name of the parent AlertLogID. With the singleton, when creating an ID, depending on the parameter values to NewID, a user may search for an existing ID and use the found id rather than creating a new one - hence if the id is created in one architecture, but coverage needs to be either checked or recorded in another architecture a look up by name can be done (again depending on the parameter values).

JimLewis commented 3 days ago

@nickg When you suggested a procedure for setting the value, are you thinking that an impure function cannot return an access type?

On p 20 for IEEE 1076-2008, I see: "It is an error if the result subtype of a function denotes either a file type or a protected type. Moreover, it is an error if the result subtype of a pure function denotes an access type or a subtype that has a subelement of an access type."

This seems to suggest that it is not an error for an impure function to return a value of an access type. Note this is not in any way an exhaustive search - I am short on time currently otherwise I would have done that.

Does NVC allow impure functions to return an access type? Due to how this code is structured, I only need this to work in NVC. The others will see it return an integer or will use a constant instead of a function call.

nickg commented 3 days ago

@JimLewis impure function would work too.

JimLewis commented 2 days ago

@nickg @Blebowski I should be able to make some updates that will facilitate this shortly. I am hoping before Monday.

Blebowski commented 2 days ago

Thanks @JimLewis that is appreciated. I will go on with integration on NVC side and rebase my OSVVM updates once the changes from you are available.

JimLewis commented 1 day ago

@Blebowski I just updated the API for Coverage. I renamed the package to CoverageVendorApiPkg and created a CoverageVendorApiPkg_NVC.vhd for you to modify. I updated the osvvm.pro file so that it compiles this file when NVC is running in the OSVVM environment.

I did the initialization using an impure function that returns integer. The intent is that you change the type and return value of this function to what you need - including an access type that returns NULL during initialization.

Note that for the initialization in the base packages, I use an integer constant instead of an impure function. One of the initializations done in there is fairly fragile in some of the tools, so I want to keep it in the domain of what currently works in their tools.

nickg / nvc

Feature Request: Functional coverage from OSVVM #1011