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.

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

VPI dirtiness tracking #5065

Merged
merged 6 commits into from
Apr 25, 2024
Merged

VPI dirtiness tracking #5065

merged 6 commits into from
Apr 25, 2024

Conversation

toddstrader
Copy link
Member

For cocotb or any other VPI users. Can be used to skip evals if callbacks happen which don't use vpi_put_value(). We've gotten decent performance gains with this. I'll post the cocotb side separately.

Copy link
Member

@wsnyder wsnyder left a comment

Choose a reason for hiding this comment

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

This seems to have a "secret handshake" with cocotb.

Is there any IEEE-specified VPI call that should be used instead?

Probably not, so, please add "///" comments to the functions describing how to be used, and also document in some appropriate point in the main user guide.

@@ -2441,6 +2447,7 @@ vpiHandle vpi_put_value(vpiHandle object, p_vpi_value valuep, p_vpi_time /*time_
return nullptr;
}
if (const VerilatedVpioVar* const vop = VerilatedVpioVar::castp(object)) {
VerilatedVpiImp::setDirty(true);
Copy link
Member

Choose a reason for hiding this comment

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

Please move after first debug print.

@toddstrader
Copy link
Member Author

toddstrader commented Apr 23, 2024

This seems to have a "secret handshake" with cocotb.

Mostly. However, other VPI users could make use of it too.

Is there any IEEE-specified VPI call that should be used instead?

I don't see anything. And I believe that makes sense since the eval-or-not decision isn't really available with other simulators.

@wsnyder
Copy link
Member

wsnyder commented Apr 23, 2024

P.S. if this is all in cocotb, why can't it track the bool itself?

@toddstrader
Copy link
Member Author

P.S. if this is all in cocotb, why can't it track the bool itself?

Yeah, I was thinking about that too. I guess I'm paranoid that someone will use the VPI w/o going through cocotb and then it won't know that the DUT has been touched.

AKA, the only component in this system that can truly know if the DUT has been disturbed via the VPI is VerilatedVpi.

@wsnyder
Copy link
Member

wsnyder commented Apr 24, 2024

I suppose cocotb could be usedin addition to other vpi/dpi extensions.

Please update docs, then I'll take this.

Comment on lines 817 to 818
static void setDirty(bool dirty) { s().m_dirty = dirty; }
static bool isDirty() { return s().m_dirty; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static void setDirty(bool dirty) { s().m_dirty = dirty; }
static bool isDirty() { return s().m_dirty; }
static void setDirty(bool dirty) { s().m_dirty = dirty; }
static bool dirty() { return s().m_dirty; }

Accessors don't usually have "is". Though occasionally break that rule.

I wonder if vpiEvalNeeded would be more descriptive than ditry?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if vpiEvalNeeded would be more descriptive than ditry?

I can go that way, however should it just be evalNeeded() since this is already in the VerilatedVpiImp class? Also, I'd then think we'd want to s/clearDirty/clearEvalNeeded and s/setDirty/setEvalNeeded. How's that sound?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Comment on lines 411 to 412
a "dirty" flag. This flag can be checked with :code:`VerilatedVpi::isDirty()`
and it can be cleared with :code:`VerilatedVpi::clearDirty()`. Used together
Copy link
Contributor

Choose a reason for hiding this comment

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

Function names need updating.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.

docs/guide/connecting.rst Outdated Show resolved Hide resolved
@@ -813,6 +814,8 @@ class VerilatedVpiImp final {
}
static void dumpCbs() VL_MT_UNSAFE_ONE;
static VerilatedVpiError* error_info() VL_MT_UNSAFE_ONE; // getter for vpi error info
static void setEvalNeeded(bool evalNeeded) { s().m_evalNeeded = evalNeeded; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static void setEvalNeeded(bool evalNeeded) { s().m_evalNeeded = evalNeeded; }
static void evalNeeded(bool evalNeeded) { s().m_evalNeeded = evalNeeded; }

@wsnyder wsnyder merged commit 25fd8ef into master Apr 25, 2024
91 checks passed
@toddstrader toddstrader deleted the vpi-dirty branch April 30, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants