[Verilog] Interface Array Testcase

[AndrewNolte]

This should be a HierarchyArrayObject, not a NonHierarchyIndexableObject. I'm not sure how we'd like to approach this. I would think it should be a cast to GPI_GENARRAY. I'm not sure what this is, there's no comments in gpi.h about it except for mentioning pseudo handles with it. Do pseudo handles (GPI_GENARRAY) mean that the handle doesn't exist in the hierarchy that we query from? Or does it just mean arrays that are in the structure?

I can't map vpiInstanceArray / vpiInterfaceArray to GPI_GENARRAY because it needs vpi_iterate with vpiRange. Indexing works however with this approach. Should I make a range iterator, and use that depending on the vpitype? Or try and get it to map these special types of GPI_ARRAY to HierarhcyObjectArray?

[ktbarrett]

I would think it should be a cast to GPI_GENARRAY.

I agree. This is a generate loop. I think an array of instances to be exactly that. An array of interfaces is very clearly an array of hierarchy objects and would map there too.

I can't map vpiInstanceArray / vpiInterfaceArray to GPI_GENARRAY because it needs vpi_iterate with vpiRange.

So the VPI assumes that everything that's a GPI_GENARRAY is a pseudo handle instead of an actual handle like vpiInstanceArray or vpiInterfaceArray. Is that's the issue? That's a really bad assumption IMO. I think we need a new object type that encodes pseudo-handle logic and a separate one that handles actual hierarchical array logic. Both map to GPI_GENARRAY, but with different implementations.

I'll have to look into the Questa and Riviera results when I get home.

[AndrewNolte]

I still don't really understand what a pseudo-region is, I think gpi.h needs a lot more clarification. My perception is it's the same as hierarchy arrays or hierarchy scopes, and sometimes the handle may be missing? However in the vpiimpl nothing in to_gpi_objtype returns GPI_GEN_ARRAY, so we only have code that treats these handles as if they're not there. Or if they are there, for convenience uses the GPI_ARRAY to avoid iterating, instead doing size/index/range discovery

[ktbarrett]

I think gpi.h needs a lot more clarification

You are absolutely correct there. I'm not certain myself how a pseudo-region works. But it is an implementation detail, not necessarily an API concern. GPI_GENARRAY should have well defined semantics regardless if the impl is a pseudo-region or an actual handle. Pseudo-regions as I understand are a hack because some simulators don't support vpiGenArrayScope or similar. The pseudo-handle is there is group the actual objects it can find, which is each of the loop's vpiGenScopes.

The entire C++ codebase needs a major facelift, but it's not public API, so it's lower on my own radar than Python rn. Sweeping C++ changes are a 3.0 issue. Until then we just need something that works, even if it's ugly as sin.

[marlonjames]

See the wiki for some details on pseudo-regions.

[marlonjames]

For VPI, there are actual differences between an instance array and gen scope array.

We need a way to distinguish hierarchy arrays like vpiInterfaceArray from things like vpiRegArray, rather than wade into the mess of GPI_GENARRAY and pseudo-regions.

[ktbarrett]

Is the difference in terms of implementation or capability? Still seems like it's an array of scopes, which is kind of what GPI_GENARRAY is.

[marlonjames]

Conceptually GPI_GENARRAY is as you say, an array of scopes. But GPI code uses it only for pseudo-regions.

In fact, vpiGenScopeArray handles are thrown away in favor of pseudo-region objects when looking up gen scope array by name:

cocotb/src/cocotb/share/lib/vpi/VpiImpl.cpp

Lines 367 to 380 in fdf7286

	/* Generate Loops have inconsistent behavior across vpi tools. A "name"
	* without an index, i.e. dut.loop vs dut.loop[0], will find a handle to
	* vpiGenScopeArray, but not all tools support iterating over the
	* vpiGenScopeArray. We don't want to create a GpiObjHdl to this type of
	* vpiHandle.
	*
	* If this unique case is hit, we need to create the Pseudo-region, with the
	* handle being equivalent to the parent handle.
	*/
	if (vpi_get(vpiType, new_hdl) == vpiGenScopeArray) {
	vpi_free_object(new_hdl);
	new_hdl = parent_hdl;
	}

We don't iterate over vpiGenScopeArray, so during iteration only vpiGenScope shows up.

When using VPI, gen scope and gen scope array objects are not instance objects like modules, packages, and interfaces are, and need to be handled differently internally.

So I think for instance arrays, we either need to differentiate GPI_ARRAY between hierarchy (instance arrays) and non-hierarchy (var array, net array), or add another GPI_* type.

Pseudo-region implementation needs an update, and maybe at that time we could merge GPI_GENARRAY into whatever type we use for other hierarchy arrays.

[ktbarrett]

Rerunning CI.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.64%. Comparing base (ed01008) to head (2b6c3a5).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3704      +/-   ##
==========================================
- Coverage   72.67%   72.64%   -0.03%     
==========================================
  Files          49       49              
  Lines        8050     8050              
  Branches     2212     2218       +6     
==========================================
- Hits         5850     5848       -2     
- Misses       1689     1695       +6     
+ Partials      511      507       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ktbarrett]

Since #3824 landed first, you'll need to update the test expectations. Use positive logic, not not verilator.

[ktbarrett]

Riviera failures.

# COUT:   3000.00ns INFO     cocotb.regression                  running test_sv_if.test_sv_intf_arr_access (4/5)
# COUT:                                                             Test that interface array objects can be accessed
# COUT:   3000.00ns ERROR    gpi                                Invalid Index - Index 1 is not in the range of [0:0]
# COUT:   3000.00ns WARNING  gpi                                Failed to find a handle at index 1 via any registered implementation
# COUT:   4000.00ns INFO     cocotb.regression                  test_sv_if.test_sv_intf_arr_access failed
# COUT:                                                         Traceback (most recent call last):
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/tests/test_cases/test_sv_interface/test_sv_if.py", line 43, in test_sv_intf_arr_access
# COUT:                                                             assert hasattr(dut.sv_if_arr[i], "a")
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/.nox/dev_test_sim-sim-riviera-toplevel_lang-verilog-gpi_interface-vpi/lib/python3.8/site-packages/cocotb/handle.py", line 772, in __getitem__
# COUT:                                                             raise IndexError(f"{self._path} contains no object at index {index}")
# COUT:                                                         IndexError: top.sv_if_arr contains no object at index 1
# COUT:   4000.00ns INFO     cocotb.regression                  running test_sv_if.test_sv_intf_arr_iteration (5/5)
# COUT:                                                             Test that interface arrays can be iterated
# COUT:   5000.00ns INFO     cocotb.regression                  test_sv_if.test_sv_intf_arr_iteration failed
# COUT:                                                         Traceback (most recent call last):
# COUT:                                                           File "/opt/actions-runner/_work/cocotb/cocotb/tests/test_cases/test_sv_interface/test_sv_if.py", line 58, in test_sv_intf_arr_iteration
# COUT:                                                             assert count == 3
# COUT:                                                         AssertionError: assert 1 == 3

[ktbarrett]

expect_error=IndexError if Riviera else ()

[AndrewNolte]

Ohh does expect_fail only cover asserts and not any general fail?

[ktbarrett]

That's correct. I'm glad you didn't know that, it's not a great system, and one we hope to replace, so users not touching them is a good thing.