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

[WIP] Virtual interfaces support #17

Closed
wants to merge 13 commits into from
Closed

[WIP] Virtual interfaces support #17

wants to merge 13 commits into from

Conversation

kozdra
Copy link

@kozdra kozdra commented Aug 29, 2022

This is more of a call for comments for now, aiming to discuss the difficulties and design of virtual interfaces support, with a proof-of-concept patchset to talk about.

A virtual interface is emitted to C++ as a variable holding a pointer to an interface scope (initialized to a null pointer), which is the most straightforward thing I came up with.

The main concern is the duality of scopes and variables in Verilator. In this patchset I decided that in the easiest case this can be worked out with minimal alteration, by simply using variables where variables are normally used (e.g. viface = iface assignments), and scopes where scopes are normally used (e.g. iface.signal or viface.signal), as opposed to MemberSel, to name one.

This comes with several drawbacks:

  • The scopes are tracked separately from their corresponding variables, and one of the two might get optimized away when still needed. I addressed this by checking for this case in V3Dead, but this might still be a potential footgun in the codebase.
  • The scopes and variables have a different representation in the verilated code (vlSymsp->TOP_unique_absolute_path_to_interface vs some_scope.some_var.path.to.viface_field):
    1. Plain interfaces, when used as variables, are emitted as pointers to their inner scopes. This is done by assigning them their inner scopes instead of parent scopes during V3Scope, and it is even quite correct.
    2. Virtual interfaces, when used as scopes, are emitted as the variable names, with an extra null check added. This does not work for every case, and is hacky. This is the most important place in which I would appreciate some feedback and ideas.
  • Variables accessed in scopes of virtual interfaces can no longer be tracked with scope identity checks (this would actually apply here anyway, even with a different implementation). This would ideally require keeping for each scope a set of scopes that it can be confused with, filled in during assignments parsing, but I solved it by setting every variable accessed through a virtual interface as public, which disables agressive optimizations on them (variable constification and assignment elimination), and works well for showcasing the proof-of-concept, as it does not impact optimizations of designs that do not use virtual interfaces. This can always be implemented separately as an optimization.

@kozdra kozdra force-pushed the virtual-interfaces branch from e8f1262 to 66fc6ca Compare August 29, 2022 15:35
@kozdra kozdra force-pushed the virtual-interfaces branch 2 times, most recently from c966e00 to a4218ee Compare September 30, 2022 13:32
Copy link

@kbieganski kbieganski left a comment

Choose a reason for hiding this comment

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

I think it's good, I don't know enough about Verilator's scopes to suggest anything better. Just the one comment, also look through the source and constify all pointers that don't change.

src/V3Scope.cpp Outdated
AstVarScope* const varscp = new AstVarScope{nodep->fileline(), scopep, nodep};
if (m_aboveCellp && m_aboveCellp->isVirtIface()) {
// Prevent optimizations to this variable: may be cross-accessed
nodep->sigPublic(true);

Choose a reason for hiding this comment

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

Is this unavoidable? Which optimization is this needed for?

Copy link
Author

Choose a reason for hiding this comment

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

This could be avoided if we added special handling for scope overlaps with virtual interfaces in V3Dead and V3Const.

Without this deoptimization the provided regression test (v8 = p8; v8.grant = 1; $display(v8.grant, p8.grant)) shows the incorrect value for the actual interface (the value access to the virtual interface gets constified, and the assignment gets eliminated). The variables are reported as unused, because each variable is in a different scope.

@kozdra kozdra force-pushed the virtual-interfaces branch 2 times, most recently from 7006b5f to 624682c Compare October 4, 2022 12:55
@kozdra kozdra force-pushed the virtual-interfaces branch from 624682c to faa21cb Compare October 12, 2022 13:34
@kozdra kozdra closed this Oct 20, 2022
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.

2 participants