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

Smart Waveform Printing #1

Closed
askvortsov1 opened this issue Jul 11, 2021 · 10 comments
Closed

Smart Waveform Printing #1

askvortsov1 opened this issue Jul 11, 2021 · 10 comments
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Comments

@askvortsov1
Copy link

Hi! We've been using this library with expect tests as per this tutorial, and it's a huge step up from having to manually inspect waveforms in Vivado! One thing we've noticed is that there's a lot of tweaking needed to get the waveform to print neatly. By any chance, have you considered a "smart print" option? One could provide:

  1. Which cycle to start on
  2. How many cycles to display
  3. Which port names to include OR which port names to exclude

And the specific widths/heights could be automatically generated based on those to show the exact desired portion of the waveform.

This would also make it easier to focus expect tests on a particular property we want to check. From what I've seen, the waveform print utils currently show all input/output waves in a circuit, which can be a bit cluttered. If an includelist/excludelist of port names is provided, one testbench function could be used for multiple test cases, each of which assesses a different expected property. For example, this file has 3 tests. In the first one, it would be nice to only show instruction and rdest. In the latter two, we don't really care about rt, rs, imm, etc: only the control signals. Additionally, an includelist could be a way to order waves in a meaningful way.

Another advantage of this proposed approach is that currently, if the display height is too small, waves could be truncated or omitted entirely without an error/warning. If display height is computed automatically, this wouldn't happen.

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Jul 12, 2021
@andrewray
Copy link

Yeah - this is an issue. I think it's hard to make a simple API function that will do the right thing in all cases, but there are a few default things I would personally change:

  • Initial wave width - I would prefer it to be smaller - maybe 2 chars per cycle by default
  • Display height auto derived, as you note
  • Display width set to something nearer 90 (which is our internal linewidth limit)

I think the display_rules argument provides a reasonable way of specifying which signals are shown (along with the order and some formatting options).

I think the approach you suggest is very likely already implementable by calculating the "right" values for various arguments to the Waveform.print function. The Waveform.Expert module provides internal access to the data that is shown and can be queried so you could calculate things like the correct waveform height and so on. You could also implement an include/exclude list yourself if you prefer it to display_rules.

Finally, you may not have tried it yet, but I recommend setting up an interactive waveform application. Waveform expect tests get you a surprisingly long way, but sometimes looking at a larger waveform and being able to scroll and scale it really helps. It's often not much extra work if you set up your test library so it both performs short-ish running expect style tests and exposes longer more extensive test options for a full testbench application that can display an interactive waveform.

@askvortsov1
Copy link
Author

askvortsov1 commented Jul 17, 2021

I think the display_rules argument provides a reasonable way of specifying which signals are shown (along with the order and some formatting options).

Ah, I missed that! That's even better than what I was thinking of. I'll try to play around with this and the Expert module.

I recommend setting up an interactive waveform application.

I've tried the following:

  • export EXPECT_TEST_WAVEFORM=1
  • Providing the serialize_to argument to Waveform.expect (instead of using Waveform.print)
  • Running dune test (with inline_tests enabled so the expect tests run)
  • Running the interactive viewer binary on generated waveform files

but it doesn't seem to be writing any files when I run dune test. Is there anything else I should configure? Sorry if I'm missing something obvious, this project is my first time working with OCaml testing.

@andrewray
Copy link

I have not tried the EXPECT_TEST_WAVEFORM flow with dune. Sounds like something might be up, as what you are doing seems like it should work.

But what I was actually suggesting, was using the Hardcaml_waveterm_interactive library. With this flow, you create an application that embeds your simulation and the waveform viewer.

Your application roughly just needs to look like

Hardcaml_waveterm_interfactive.run waves

where waves is the same thing you would pass to Hardcaml_waveterm.print. You can then useful add command-line arguments to set things like display rules - in particular with hierarchy you can run the application to look at various modules individually or together as you debug.

@askvortsov1
Copy link
Author

Hardcaml_waveterm_interfactive.run waves

This works!

in particular with hierarchy you can run the application to look at various modules individually or together as you debug

We got stuck on this though. Looking through the source code, I'm assuming that the process is more-or-less as follows:

  • Create Scope.t with ~auto_label_hierarchical_ports:true, ~trace_properties:true, and ~flatten_design:true (as it won't simulate if not flattened)
  • The simulation engine should log internal ports as grandparent$parent$port_name
  • The widget splits port names on that $ delimiter to build up a tree representing the module hierarchy
  • Keyboard controls can then be used to navigate across nodes in the tree

We're getting stuck on generating internal ports: whenever we launch the widget, we only see ports specified in the I and O interfaces of the top level module we're simulating. In addition to the above config when creating Scope.t, we've also tried using a display rule that explicitly shows everything via regex:

Hardcaml_waveterm_interactive.run ~display_rules:[
    Display_rule.port_name_matches (Re.Posix.compile (Re.Posix.re ".*")) ~wave_format:Unsigned_int
  ] waves

and Expert.Sim.wrap sim instead of Waveform.create sim, but we're still only seeing top-level I and O ports.

Is there anything we might be missing?

@andrewray
Copy link

in particular with hierarchy you can run the application to look at various modules individually or together as you debug

We got stuck on this though. Looking through the source code, I'm assuming that the process is more-or-less as follows:

  • Create Scope.t with ~auto_label_hierarchical_ports:true, ~trace_properties:true, and ~flatten_design:true (as it won't simulate if not flattened)

trace_properties is not needed.

  • The simulation engine should log internal ports as grandparent$parent$port_name

When you create the simulation, you decide what internal stuff to expose. In the latest code there is a config argument you can set to trace everything. Which Hardcaml version are you using?

  • The ### ### widget splits port names on that $ delimiter to build up a tree representing the module hierarchy
  • Keyboard controls can then be used to navigate across nodes in the tree

We're getting stuck on generating internal ports: whenever we launch the widget, we only see ports specified in the I and O interfaces of the top level module we're simulating. In addition to the above config when creating Scope.t, we've also tried using a display rule that explicitly shows everything via regex:

The display rules can only show the things that are exposed from the simulator. I suspect this is the problem you are seeing.

Aside - why? There can be a lot of internal ports, especially as designs grow. This has a measurable impact on performance as it requires a lot of copying. But that said, until you care about performance, you should just make the simulator expose everything.

Hardcaml_waveterm_interactive.run ~display_rules:[
    Display_rule.port_name_matches (Re.Posix.compile (Re.Posix.re ".*")) ~wave_format:Unsigned_int
  ] waves

and Expert.Sim.wrap sim instead of Waveform.create sim, but we're still only seeing top-level I and O ports.

You shouldn't need the expert module for this.

@askvortsov1
Copy link
Author

The display rules can only show the things that are exposed from the simulator. I suspect this is the problem you are seeing.

Yeah, I think this is exactly the issue. We couldn't find config arguments to expose internal signals, so we assumed that configuration should occur in the waveform print / viewer.

In the latest code there is a config argument you can set to trace everything. Which Hardcaml version are you using?

We're on the latest bleeding release, v0.15~preview.124.35+330. The config fields we found for Cyclesim.With_interface.create were is_internal_port, combinational_ops_database, compute_digest, and deduplicate_signals, but none of those felt like what we were looking for. Circuit.Config.t didn't seem relevant either. Is there another configuration input somewhere?

@andrewray
Copy link

is_internal_port is the one you want. It is basically passed every signal inside the design, and if the function returns true a special port is made for it so it can be accessed from outside (ie traced in a waveform).

But actually, the way to do this is to pass the config argument as Cyclesim.create ~config:Cyclesim.Config.trace_all.

We haven't properly documented this, so we'll add it to our stack to improve matters.

@askvortsov1
Copy link
Author

askvortsov1 commented Jul 23, 2021

is_internal_port is the one you want. It is basically passed every signal inside the design, and if the function returns true a special port is made for it so it can be accessed from outside (ie traced in a waveform).

Ah, that makes sense. For some reason, I interpreted that as a setting for hiding signals. Maybe expose_internal_port would be clearer?

But actually, the way to do this is to pass the config argument as Cyclesim.create ~config:Cyclesim.Config.trace_all.

It works! We just used it to debug some issues, and this has been the nicest hardware debugging experience I've had: definitely beats out managing and scrolling through Vivado simulation waveforms.

One suggestion: collapsing nodes in the hierarchy is great for filtering down what's shown on screen, but there's still a decent amount of information shown. Display rules (and apparently is_internal_port) can be used to filter down the listed waveforms, but that requires rebuilding and relaunching the viewer. What if you could double click a signal name, and that would toggle whether that signal is displayed in the output? For example:

image

This would allow quickly narrowing down displayed information to make tracing easier. An "expand all" button/keyboard shortcut might also make sense if this is implemented.

@andrewray
Copy link

I do have some plans for making this thing easier to configure. But frankly, the waveform viewer is now really old and crufty, and needs some love and attention. A lot of the issues you raise I think would be best supported by supplying a more flexible API for interacting with the viewers components so you can use it in the way you see fit.

No promises on timescale though.

@andrewray
Copy link

I think many things here are resolved. Any more major changes will come with a new version of the tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

No branches or pull requests

3 participants