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 systemtf #1470

Closed
veripoolbot opened this issue Jun 20, 2019 · 15 comments
Closed

VPI systemtf #1470

veripoolbot opened this issue Jun 20, 2019 · 15 comments
Labels
effort: hours Expect this issue to require roughly hours of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@veripoolbot
Copy link
Contributor


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1470 from https://www.veripool.org

Original Assignee: Stefan Wallentowitz (@wallento)


Hi,

VPI allows to add and overwrite System Tasks and Functions. I propose to add the infrastructure to allow for that in the following way:

  • Add command line switch (e.g., --vpi-systemtf) to enable this functionality
  • Let Verilator not emit implementation of the systemtf, but instead emit some kind of function table with pointers to the configured implementation, along with defaults and unresolved systemtf if needed. Furthermore emit calls into the runtime that implements this.
  • Implement the VPI part by accessing this function table infrastructure
  • Runtime functions that implement the calls

Enabling this feature has a negative impact on performance, that's why it should definitely be optional.

I gave a very rough estimate in hours, happy to pick it up once I find time.

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-20T13:33:15Z


Note Verilator does support a language extension where arbitrary doller-user functions can instead be converted into DPI functions.

This suggests a refinement of what you suggest, namely Verilator would spit out automatically code that converted DPI calls into old-school VPI calls.

Another complication is verilator must know the data types of the user's functions. This is usually done by the simulator at verilation time needing to call into the user VPI routines/tables. This will be a pain.

Having these VPI extensions would be a fine addition, but I wonder if this is really in the service of users runtime wise? Could cocotb switch to generating DPI functions? This is more modern, far cleaner, and would speed up cocotb not just with Verilator but with every simulator.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-06-20T13:41:13Z


It is actually even worse: It only uses the systemtf to re-direct $display-style system functions to its output.

As you say, undergoing the full exercise seems like a massive task just for this minor part of the complexity. But I cannot come up with an acceptable workaround for that so far. I think currently, the existing implementations all just turn the Unimplemented error of registering systemtf into an empty function..

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-06-20T13:44:32Z


How would you fancy an implementation that only allows to overwrite systemtf for a limited set of systemtf (in particular the ones mentioned above)?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-20T14:04:56Z


Adding a way to overload $display would be ok but you'd have a larger problem that display takes arguments and the DPI doesn't support vaargs. Plus then you'd need $write, etc, etc.

Knowing it's just display helps enormously. I suggest using VPI was just a hack to get around that other simulators didn't have what was really wanted, namely an output-stream override. And Verilator has better alternative paths.

  1. Simplest is just -DVL_PRINTF=my_printf at compile time, all output go to a new function. You're done. Does this work?

  2. See t_dpi_display.v. This defines a DPI function with printf style format. Then add a new way to override $display instead of the new name (as you suggested). This is better than VPI as Verilator understands this takes the %formats and compresses it all to a single pass-a-string call.

  3. Add a new attribute that specifies a DPI function that replaces the downstream VL_PRINTF with a DPI function taking a string.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-06-20T14:07:39Z


Ah, you are absolutely right. Good, that I didn't start trying to implement anything so far :)

I think we should be able to solve that on the cocotb side to reduce the complexity. Will have a look into it next week!

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-19T21:00:29Z


Hi,

sorry, took much longer as I hoped to even look into it. Attached please find a first, rather trivial patch, that allows us to define vpi_register_systf in our Verilator-specific cocotb infrastructure and thereby have no impact on the common code. A change there would also not be overly complex, but I would like to get your comment on whether this can be accepted from your point of view. I see no negative implications, and it is probably the only strange workaround (which is great :)

Cheers,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-19T22:21:50Z


Weak seems somewhat hacky, but I can live with hacky. You do however need to do not break non-GCC compilers e.g. MSVCC. Please use e.g. PLI_DLLWEAK then declare that similar to how PLI_DLLISPEC is made.

A search turned up this MSVC++ somewhat equivalent to weak: https://stackoverflow.com/questions/2290587/gcc-style-weak-linking-in-visual-studio

however it might be safer just to set it to empty for MSVCC (the conditions similar to those that test to make PLL_DLLESPEC.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-20T04:25:20Z


Thanks, Wilson, I was already thinking about the issue with other compilers but only came up with LLVM :) I think it is easier to handle this minor issue on the cocotb side, because it is cleaner there (define out the call for Verilator).

Time to get to the actual stuff then, so please keep the issue open, thanks!

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-20T14:30:23Z


Okay, here is one that is equally hacky and elegant on both sides. Instead of making the symbol weak, how about just not error'ing on unimplemented functions. The application may choose to deliberately not have it error out, then use common code to probe for existence, as per the patch attached. An improvement would be to only do it for ones that allow probing (need to check), but register_systf allows it by returning 0. But ultimately, the application developer is fully aware of this and can track on its own because otherwise they wouldn't have set -DVL_SUPPRESS_UNIMP during the compilation of the runtime. Seems like a simple "compromise" in terms of hackiness on both sides.

We can still fallback on the cocotb-side solution which requires larger #ifdef VERILATOR areas in the common code which is considered unfavored (but acceptable).

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-20T15:32:40Z


Perhaps when suppressed this should be setting the error return code so vpi_chk_error returns something sane e.g. level=vpiInternal message=Unsupported...?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-20T15:40:58Z


Wilson Snyder wrote:

Perhaps when suppressed this should be setting the error return code so vpi_chk_error returns something sane e.g. level=vpiInternal message=Unsupported...?

Awesome, thanks! Always new sections in the LRM to learn about :) Will get onto that over the weekend!

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-26T12:39:19Z


The first part is much easier than I thought, because @Verilated::fatalOnVpiError(false);@ does the job. Now I need to work on defining output, so please keep it open, thanks!

@veripoolbot veripoolbot added effort: hours Expect this issue to require roughly hours of invested effort to resolve type: feature-IEEE Request to add new feature, described in IEEE 1800 labels Dec 22, 2019
@wsnyder
Copy link
Member

wsnyder commented Mar 6, 2020

@wallento is this issue still appropriate for cocotb or can we punt this until the nebulous "future"? Thanks.

@wallento
Copy link
Member

It is not an urgent requirement for cocotb, but useful in future as you suggest (I believe it will happen in 2020).

@wsnyder wsnyder added the resolution: abandoned Closed; not enough information or otherwise never finished label Dec 13, 2020
@wsnyder
Copy link
Member

wsnyder commented Dec 13, 2020

The use case for this was resolved earlier. We'll wait for a new request for the more general VPI, but really should encourage DPI wherever possible as is a faster interface.

@wsnyder wsnyder closed this as completed Dec 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: hours Expect this issue to require roughly hours of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Development

No branches or pull requests

3 participants