[vpi] Structure test case #3781
Conversation
AndrewNolte
force-pushed
the
vpi-structure-tc
branch
4 times, most recently
from
8b37f46
to
b1c9f40
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3781 +/- ##
==========================================
- Coverage 73.54% 72.70% -0.84%
==========================================
Files 49 49
Lines 7916 8050 +134
Branches 2195 2208 +13
==========================================
+ Hits 5822 5853 +31
- Misses 1581 1682 +101
- Partials 513 515 +2 ☔ View full report in Codecov by Sentry. |
|
I'd like to land this before putting back up the vpi iteration fixes for verilator, the last of those PRs is almost landed: verilator/verilator#4965 |
|
Any update on this? verilator/verilator#4965 was landed recently @ktbarrett |
|
I've been very busy with my move to Colorado. I probably won't have time to work on cocotb until next week at the earliest. My question is why do we need to add a new test and source code for this? It does something g very similar to test_iteration and test_discovery (albeit in a lot better way IMO). This is just more code that's going to have to be refactored eventually when someone takes a cracked at replacing all discovery-type and value access tests with a more holistic approach. That refactor was next on my radar after my current PRs are in. |
|
I guess I'm fine with this coming in if it's blocking your other work. Just know it will probably be rm -rf'd in the near future. |
|
I agree, these tests try to achieve something similar. I initially just wanted to measure the progress on verilator vpi, so didn't thinking recreating this on sample_module or endian_swapper would be worth my time due to expected ci breaking of other tests. I think we should move towards golden files for a lot of the cocotb tests for a few reasons:
I think this provides a start for moving more of the structure / simulator differentiating tests to use golden files |
Co-authored-by: Marlon James <marlon.james@gmail.com>
Co-authored-by: Marlon James <marlon.james@gmail.com>
ktbarrett
force-pushed
the
vpi-structure-tc
branch
from
96ee107
to
2dd7e15
Compare
|
Questa failure. |
| @cocotb.test( | ||
| skip=SIM_NAME.startswith("riviera"), | ||
| expect_error=DirectLenMismatch | ||
| if SIM_NAME.startswith(("icarus", "verilator", "xmsim", "modelsim")) | ||
| else (), | ||
| ) |
There was a problem hiding this comment.
So only GHDL and NVC work here. Interestingly, they are VHDL-only simulators, I wonder if there's some reason for that? EDIT: Big dumb. This is Verilog only. So ALL simulators fail this test.
Is there really any reason to test or enforce this? Why do we care if iteration handles and access by name handles being the same? Is there some reason to not just remove this test?
|
@marlonjames @AndrewNolte @ktbarrett: Verilator though, using cocotb latest main and verilator 5.022 is not elaborating the struct members. @marlonjames is this expected? I had thought you fixed this in past PRs - in all your work in all the comments and PRs linked here: #1275 |
No. As stated in the issue you linked, this is a Verilator issue, and moreover a IEEE standards issue. Indexing into packed objects, structure or array, through the VPI is probably not standards compliant behavior (I'd have to find the exact section). Additionally, indexing into packed arrays doesn't work in any simulator (AFAIK), so any kind of complex packed data structure such as a packed array of packed structures can't be indexed into; only the very trivial case of a top-level packed struct. #3608 attempts to allow the behavior in the other direction: getting the value of a top-level packed structure as a single value. Regardless of how that PR falls out, if you want to write portable standards compliant code, I'd stop relying on the ability to index into packed structures. |
|
Thanks for getting back to me. If you follow all the links to understand the breadth of the work, you will see mentioned that @marlonjames also closed out issues in verilator/verilator. I was aware this would start as a verilator issue, then if resolved in verilator become a cocotb issue. For example, there is confirmation of this here: #3237
There is a wide net of linking from issue to issue, so I was having trouble finding the definitive answer to the question I had posted, so I thought to reach out. I now see some more explicit evidence in the makefiles that verilator does not support struct constructs in the portmap/port signature.
@ktbarrett if you have a moment could you please point me to the standards that you are referencing here: |
|
@ian-l-kennedy You can simply remove the |
|
@ktbarrett if you have a moment could you please point me to the standards that you are referencing here: Regardless of how that PR falls out, if you want to write portable standards compliant code, I'd stop relying on the ability to index into packed structures.. I am especially interested in removing SystemVerilog code constructs from my synthesizable code. Especially ones that are "against the grain" of any portability standard. |
|
Hmmm. So IEEE 1800-2017 section 37.18 states that we should be able to use I wonder how it's supposed to work with getting/setting values. The standard doesn't mention anything wrt to that =/ |
Uses golden files to easily compare verilog symbol structure between simulators. Split off from #3756