Simplify VpiArrayObjHdl::initialise #3624

AndrewNolte · 2024-01-03T00:39:22Z

I had issues with range_idx being calculated incorrectly, so I wanted to see what simulators actually require this, if at all. It gets calculated incorrectly when using get_handle_by_name() with a fully qualified name (not from handle.py) with brackets in other parts of the path. I didn't see range_idx being actually used in testing on Xcelium. It doesn't look like there were any tests when it was put in 54d2965

codecov · 2024-01-03T20:25:47Z

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 72.67%. Comparing base (ed01008) to head (6c53e80).

Files	Patch %	Lines
src/cocotb/share/lib/vpi/VpiObj.cpp	76.47%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3624   +/-   ##
=======================================
  Coverage   72.67%   72.67%           
=======================================
  Files          49       49           
  Lines        8050     8044    -6     
  Branches     2212     2201   -11     
=======================================
- Hits         5850     5846    -4     
+ Misses       1689     1678   -11     
- Partials      511      520    +9

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

ktbarrett

Does this fix your issue simply because only Questa/ModelSim, which you aren't using, need pseudo-regions, which you've segregated into ifdef's? So the problem of supporting get_handle_by_name from the root on Questa, if there are multiple indexes, is still present, correct? But this commit removes this logic, which isn't necessary for the other simulators?

What tests exist in the regression which test this?

@marlonjames Do you have any concerns here?

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

AndrewNolte · 2024-01-04T01:20:51Z

s_vpi_value

Yeah we have use cases where we query objects from the root handle, but the logic for searching through the brackets causes issues. Are we okay with adding tests specifically for the simulator module? I think a good test case would be iterating through handles in a large design and asserting that subhdl == roothdl.get_handle_by_name(subhdl._path), and can be skipped on questa for now.

ktbarrett · 2024-01-04T03:53:23Z

Are we okay with adding tests specifically for the simulator module?

Oh absolutely! I encourage it.

can be skipped on questa for now.

That's fine. We can maybe open an upstream ticket or at least track it in an issue if you could open that with the particulars that I'm not yet groking.

AndrewNolte · 2024-01-04T15:21:11Z

I added it to the iteration test case, but ideally it would test on all of the designs, with multiple arrays in the path. Also, while looking at the iteration tests, I think I found that these test tags were removed on accident? db1cce7#diff-1082c9445dd85ec000286580d8e757077f81782e15cd253a1279b956a3cad2cb

marlonjames · 2024-01-04T18:25:29Z

I will try to take a look at this soon.

ktbarrett · 2024-01-05T19:20:35Z

Unfortunately I don't think there's anything we can do until the private CI job hang is resolved. So sorry if we don't look at this sooner.

imphil · 2024-01-10T21:04:00Z

Unfortunately I don't think there's anything we can do until the private CI job hang is resolved. So sorry if we don't look at this sooner.

That's resolved now, sorry for the disruption.

tests/test_cases/test_iteration_verilog/test_iteration_es.py

ktbarrett · 2024-01-16T15:31:56Z

@marlonjames Could you have a look?

marlonjames · 2024-01-16T02:11:52Z

tests/test_cases/test_iteration_verilog/test_iteration_es.py

@@ -52,25 +53,35 @@ async def recursive_discovery(dut):
    else:
        pass_total = 265

+    # Icarus doesn't support array indexes like get_handle_by_name("some_path[x]")
+    SKIP_HANDLE_ASSERT = cocotb.SIM_NAME.lower().startswith(("riviera", "icarus")) or cocotb.LANGUAGE in ["vhdl"]


Suggested change

SKIP_HANDLE_ASSERT = cocotb.SIM_NAME.lower().startswith(("riviera", "icarus")) or cocotb.LANGUAGE in ["vhdl"]

SKIP_HANDLE_ASSERT = SIM_NAME.startswith(("riviera", "icarus"))

Makefile skips this test for VHDL, and SIM_NAME is already defined above.

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

marlonjames · 2024-01-16T22:16:55Z

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

    m_indexable = true;

    int range_idx = 0;

-    /* Need to determine if this is a pseudo-handle to be able to select the
-     * correct range */
+#ifdef MODELSIM


std::string hdl_name = vpi_get_str(vpiName, hdl); should be pulled out of the #ifdef, and the non-Modelsim case should enforce that hdl_name == name so that we get an error if there is another VPI simulator that needs pseudo-regions for multi-dimensional arrays.

I'm don't think name != hdl_name always implies this? In Verilator when there's a generate scope, hdl_name = 'my_mod.scope' and name = 'scope', but maybe that's another issue for fix on the Verilator side...

vpiName should be the local name of the object, vpiFullName should be the full hierarchical name.

Because pseudo-regions use the handle of the parent (there is no proper VPI object with a handle), the names should mismatch.
That is how it works now. Pseudo-regions should be re-worked to preserve more context, right now the checking for and handling of pseudo-regions is scattered across the GPI codebase.

Maybe this should be a follow on then? One thing at a time with a PR. Maybe open an issue for future investigation.

Yeah I think verilator needs a test case for just iterating through the tree and printing the handle info. Maybe we can add that to cocotb to measure simulator vpi compatibility, since the infrastructure for running different sims is already there? Or maybe that would belong on https://chipsalliance.github.io/sv-tests-results/

The original issue for me was that in create_gpi_obj_from_handle, name is assumed to be the vpiName of the object, but it's not the case if calling the get_handle_by_name from a handle that isn't the direct parent handle. Would it be possible to add some flag there to not create pseudo handles, or not create them if we're querying down further than just the children?

Hey following up on this, if the pseudo handle check here is only needed for modelsim, wouldn't we rather have it only create handles for that sim?

Also, I made the vpi dump verilator test case, and it looks like the scopes are generated, so we may just need to add pseudo-handle code specific to verilator https://github.com/verilator/verilator/pull/4838/files

Ok, I think I have a better understanding now.
So your problem came from passing a hierarchy to SimHandleBase._handle.get_handle_by_name() rather than just the name of a direct child object.

There is no analysis of the name passed into native_check_create(), it is just concatenated with the name of the parent handle and looked up via vpi_handle_by_name(). The possibly-a-hierarchy name is then passed through to create_gpi_obj_from_handle(), which also has the assumption that it is the name of the object. Finally, that is passed to initialise() as if it is the name.

I think we need to either fix the code making the assumptions, or provide a separate function to call when looking up a handle that is not a direct child (e.g. we recommend using _id() with a hierarchical path for some cases where simulators have problems with generates, etc, but that could hit this problem as well).

Yeah I did some more testing and this ifdef should actually be included. The one below does seem to be verify specific to modelsim though

marlonjames · 2024-03-08T22:56:38Z

I'll take another look at this soon. Please don't merge until then.

ktbarrett · 2024-04-21T21:53:58Z

Re-running CI.

ktbarrett · 2024-04-30T04:39:46Z

tests/test_cases/test_iteration_verilog/test_iteration_es.py

+    # Icarus doesn't support array indexes like get_handle_by_name("some_path[x]")
+    SKIP_HANDLE_ASSERT = cocotb.SIM_NAME.lower().startswith(
+        ("riviera", "icarus")
+    ) or cocotb.LANGUAGE in ["vhdl"]


cocotb.LANGUAGE was removed.

ktbarrett

LGTM. @marlonjames?

marlonjames · 2024-06-06T15:07:09Z

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

        }
-
+#endif
+        vpi_free_object(iter);  // Need to free iterator since exited early


If rangeHdl is null, the iterator handle is no longer valid so must not be freed here.

marlonjames · 2024-06-06T15:17:42Z

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


-        while ((rangeHdl = vpi_scan(iter)) != NULL) {
-            if (idx == range_idx) {
+#ifdef MODELSIM


This code isn't Modelsim specific.
The problem in recursive_discovery test has to do with dut._handle.get_handle_by_name(subpath) as detailed in #3710.

AndrewNolte force-pushed the sim-arr-simplify branch from e8d01a5 to 4177293 Compare January 3, 2024 20:06

ktbarrett reviewed Jan 3, 2024

View reviewed changes

src/cocotb/share/lib/vpi/VpiCbHdl.cpp Outdated Show resolved Hide resolved

marlonjames added the category:codebase:gpi relating to the GPI or one of the implementations label Jan 3, 2024

ktbarrett mentioned this pull request Jan 4, 2024

Add test decorator back #3636

Merged

AndrewNolte force-pushed the sim-arr-simplify branch from 5df1c83 to 183f539 Compare January 4, 2024 21:58

ktbarrett reviewed Jan 10, 2024

View reviewed changes

tests/test_cases/test_iteration_verilog/test_iteration_es.py Outdated Show resolved Hide resolved

AndrewNolte force-pushed the sim-arr-simplify branch 2 times, most recently from cea9cb5 to e1ce4ed Compare January 12, 2024 22:36

ktbarrett force-pushed the sim-arr-simplify branch 2 times, most recently from d5c234e to 3602f6a Compare January 16, 2024 07:27

ktbarrett approved these changes Jan 16, 2024

View reviewed changes

marlonjames reviewed Jan 16, 2024

View reviewed changes

marlonjames mentioned this pull request Jan 18, 2024

Allow indexing to vpiscope objects #3664

Closed

AndrewNolte mentioned this pull request Jan 18, 2024

Vpi dump module test case verilator/verilator#4838

Merged

marlonjames mentioned this pull request Feb 8, 2024

[GPI] gpi_get_handle_by_name() implementations don't handle passing hierarchical path as subhandle name #3710

Open

AndrewNolte force-pushed the sim-arr-simplify branch from 3e7303f to a52b88b Compare March 7, 2024 22:48

ktbarrett closed this Apr 21, 2024

ktbarrett reopened this Apr 21, 2024

ktbarrett reviewed Apr 30, 2024

View reviewed changes

simplify vpi array init

dd417fe

fix language

6c53e80

AndrewNolte force-pushed the sim-arr-simplify branch from a52b88b to 6c53e80 Compare April 30, 2024 20:44

ktbarrett approved these changes May 1, 2024

View reviewed changes

ktbarrett requested a review from marlonjames June 6, 2024 02:59

marlonjames reviewed Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify VpiArrayObjHdl::initialise #3624

Simplify VpiArrayObjHdl::initialise #3624

AndrewNolte commented Jan 3, 2024

codecov bot commented Jan 3, 2024 •

edited

ktbarrett left a comment

AndrewNolte commented Jan 4, 2024

ktbarrett commented Jan 4, 2024

AndrewNolte commented Jan 4, 2024

marlonjames commented Jan 4, 2024

ktbarrett commented Jan 5, 2024

imphil commented Jan 10, 2024

ktbarrett commented Jan 16, 2024

marlonjames Jan 16, 2024

marlonjames Jan 16, 2024

AndrewNolte Jan 18, 2024

marlonjames Jan 18, 2024

marlonjames Jan 18, 2024

ktbarrett Jan 18, 2024

AndrewNolte Jan 18, 2024

AndrewNolte Jan 18, 2024

AndrewNolte Jan 23, 2024

marlonjames Jan 25, 2024 •

edited

AndrewNolte Mar 7, 2024

marlonjames commented Mar 8, 2024

ktbarrett commented Apr 21, 2024

ktbarrett Apr 30, 2024

ktbarrett left a comment

marlonjames Jun 6, 2024

marlonjames Jun 6, 2024

	SKIP_HANDLE_ASSERT = cocotb.SIM_NAME.lower().startswith(("riviera", "icarus")) or cocotb.LANGUAGE in ["vhdl"]
	SKIP_HANDLE_ASSERT = SIM_NAME.startswith(("riviera", "icarus"))

Simplify VpiArrayObjHdl::initialise #3624

Are you sure you want to change the base?

Simplify VpiArrayObjHdl::initialise #3624

Conversation

AndrewNolte commented Jan 3, 2024

codecov bot commented Jan 3, 2024 • edited

Codecov Report

ktbarrett left a comment

Choose a reason for hiding this comment

AndrewNolte commented Jan 4, 2024

ktbarrett commented Jan 4, 2024

AndrewNolte commented Jan 4, 2024

marlonjames commented Jan 4, 2024

ktbarrett commented Jan 5, 2024

imphil commented Jan 10, 2024

ktbarrett commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlonjames Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlonjames commented Mar 8, 2024

ktbarrett commented Apr 21, 2024

Choose a reason for hiding this comment

ktbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2024 •

edited

marlonjames Jan 25, 2024 •

edited