Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Vpi Generate Scopes #4913

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Fix Vpi Generate Scopes #4913

merged 1 commit into from
Mar 6, 2024

Conversation

AndrewNolte
Copy link
Contributor

This gets us a lot closer to parity with Xcelium / Icarus, and only adds extra verilated code if vpi is turned on.

Addresses #3609
Depends on #4838

  • small refactors:
    • move common logic for name/fullname to VerilatedVpioScope
    • move top detection logic to VerilatedVpioModule (I think we can use hier[0] to detect this instead)
  • emit SCOPE_OTHER objects in v3emit
  • Add vpiInternalScope case to vpi_iterate, which returns the new VerilatedVpioScopeIter that cycles through both vpiGenScope and vpiModule objects

@AndrewNolte AndrewNolte force-pushed the anolte/fix-vpi-scopes branch from 2b7fb08 to 8eee72b Compare February 21, 2024 22:23
@AndrewNolte AndrewNolte mentioned this pull request Feb 21, 2024
Closed
wsnyder
wsnyder reviewed Feb 22, 2024
View reviewed changes
include/verilated_vpi.cpp Outdated Show resolved Hide resolved
include/verilated_vpi.cpp Outdated Show resolved Hide resolved
include/verilated_vpi.cpp Outdated Show resolved Hide resolved
include/verilated_vpi.cpp Show resolved Hide resolved
include/verilated_vpi.cpp Outdated Show resolved Hide resolved
@AndrewNolte AndrewNolte changed the title Fix Vpi Scopes Fix Vpi Generate Scopes Feb 22, 2024
@AndrewNolte AndrewNolte force-pushed the anolte/fix-vpi-scopes branch 7 times, most recently from 6ad2a78 to 5866d99 Compare February 22, 2024 23:14
@AndrewNolte AndrewNolte mentioned this pull request Feb 23, 2024
Closed
@AndrewNolte
Copy link
Contributor Author 

%Warning: vlt/t_time_vpi_1ms10ns: Line 15 miscompares; obj_vlt/t_time_vpi_1ms10ns/vlt_sim.log != t/t_time_vpi_1ms10ns.out
F1: t vpiSimTime = 0,6000000  vpiScaledRealTime = 60
F2: top.t vpiSimTime = 0,6000000  vpiScaledRealTime = 60

Just "t" here is more correct, right? I think this was because VerilatedVpioScope name() was updated to match how it's handled for module

@wsnyder
Copy link
Member

wsnyder commented Feb 29, 2024

Check with other simulators but yes I think vpiName should be "t", vpiFullName "top.t".

@AndrewNolte AndrewNolte force-pushed the anolte/fix-vpi-scopes branch 2 times, most recently from 00b7620 to 2ccde93 Compare March 4, 2024 16:37
This was referenced Mar 4, 2024
Merged
Closed
Merged
@AndrewNolte AndrewNolte force-pushed the anolte/fix-vpi-scopes branch from 2ccde93 to 0acc0ec Compare March 5, 2024 21:54
AndrewNolte
AndrewNolte commented Mar 5, 2024
View reviewed changes
test_regress/t/t_vpi_dump.iv.out
@@ -206,16 +134,6 @@ t (vpiModule) t
t.outer_scope[2].inner_scope[1].arr.LENGTH (vpiParameter) t.outer_scope[2].inner_scope[1].arr.LENGTH
vpiConstType=vpiBinaryConst
t.outer_scope[2].inner_scope[2] (vpiGenScope) t.outer_scope[2].inner_scope[2]
vpiModule:
t.outer_scope[2].inner_scope[2].arr (vpiModule) t.outer_scope[2].inner_scope[2].arr
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was repeated (vpiModule and vpiInternalScope), now just vpiInternalScope is needed.

AndrewNolte
AndrewNolte commented Mar 5, 2024
View reviewed changes
test_regress/t/t_vpi_dump.cpp
@@ -38,7 +38,7 @@ std::map<int32_t, std::vector<int32_t>> iterate_over = [] {
};

std::vector<int32_t> module_options = {
vpiModule, // Aldec SEGV on mixed language
// vpiModule, // Aldec SEGV on mixed language
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed for iteration now that vpiInternalScope is supported

AndrewNolte
AndrewNolte commented Mar 5, 2024
View reviewed changes
include/verilated_vpi.cpp
@@ -1978,13 +2004,21 @@ vpiHandle vpi_iterate(PLI_INT32 type, vpiHandle object) {
return ((new VerilatedVpioVarIter{vop, true})->castVpiHandle());
}
case vpiModule: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case vpiModule is the child type we want to iterate over, not the parent type. So any scope should be valid for iteration

AndrewNolte
AndrewNolte commented Mar 5, 2024
View reviewed changes
include/verilated_vpi.cpp
if (i < scopename.length()) pos = i++;
}
}
if (VL_UNLIKELY(pos == std::string::npos)) m_toplevel = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a module can be top level, so I moved this down a class

@AndrewNolte
Copy link
Contributor Author

This should be ready to merge as well now

@wsnyder wsnyder merged commit 6db149c into verilator:master Mar 6, 2024
39 of 40 checks passed
@wsnyder wsnyder mentioned this pull request Apr 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wsnyder wsnyder wsnyder left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndrewNolte @wsnyder