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

Change the simulation harness to not use readmemh and writememh #1603

Open
rachitnigam opened this issue Jul 16, 2023 · 8 comments
Open

Change the simulation harness to not use readmemh and writememh #1603

rachitnigam opened this issue Jul 16, 2023 · 8 comments
Labels
C: Simulation Changes for the simulation backend code

Comments

@rachitnigam
Copy link
Contributor

We've had ongoing problems with really understanding how to handle @external cells and their ports. For example, #1034 tracks a slew of issues where we either keep attempting to add back @clk and @reset ports from these memories or remove them because they behave incorrectly for the synthesis backend.

I think the right thing to do here is completely getting rid of the magical readmemh and writememh functions we emit for the simulation backend and forcing ourselves to use the externalized signature of the top-level component directly.

Concretely, this would mean:

  • The externalize pass runs for the default compilation pipeline
  • We change the test harness to somehow read and write in values for all the memories manually

I think this will provide clarity on issues like #1034 as well since it'll force us to use our designs in a that our users are having to.

@rachitnigam rachitnigam added Status: Discussion needed Issues blocked on discussion C: Simulation Changes for the simulation backend code labels Jul 16, 2023
@sampsyo
Copy link
Contributor

sampsyo commented Jul 17, 2023

Hmm; this is an interesting point. Here are two very tentative thoughts, which are kind of connected:

  1. It would be nice to keep around the readmemh/writememh option as one possible backend because it is going to be much faster than the alternative for large memories. To be explicit, the alternative is to have the (probably C++) Verilator harness fopen a file, loop over its contents, and on every iteration, issue a memory write (by setting the address/enable/value ports and waiting for the write to complete). This is a lot slower than just instructing the simulator to memcpy some bytes into its internal data storage for the array. (In fact, this came up in the context of discussing Proposal: Calyx-to-FIRRTL backend to use ESSENT #1552 with @sbeamer; ESSENT does not yet support readmemh-like functionality, but it would like to someday for this reason.) So there would be value in preserving both options and testing them both.
  2. (Even less certain about this one; it's probably a bad idea, but putting it here anyway for completeness:) We could unify this with the AXI wrapper. That is, if the AXI wrapper worked by interacting with the externalized version of the main component, then our tests for the AXI interface would also test this functionality.

Anyway, it's definitely a good idea to be able test the "real" external interface in the way you describe!

@rachitnigam
Copy link
Contributor Author

So, I thought our AXI tests already do (2) since the AXI wrapper generator needs to pass in memories by reference

@sampsyo
Copy link
Contributor

sampsyo commented Jul 17, 2023

Ah, taking a glance at the generated Verilog, I think you're right. (For some reason, I had it in my head that it was replacing the externalized memories with special AXI-ified memories; not sure where I got that idea.) It might be good idea to assess whether testing this AXI wrapper would suffice to test what we want… (not that a simple non-AXI test harness for the externalized interface wouldn't be a good idea too).

@rachitnigam
Copy link
Contributor Author

Okay, here is an actionable approach to this:

  1. By default, the Calyx compiler will always emit the fully "externalized" version of the top-level component.
  2. We have another tool, kept outside the compiler source, that wraps things nicely with a module capable to generating the current readmemh and writememh code. This is similar to how we want to eventually think about the AXI and XML backends (Seperate out AXI and XML generator tools #1084)

This way, we have created a clear interface for what the Calyx compiler is responsible for and what the "simulation wrapper generator" is responsible for. This will also allow us to explain the existence of wrap-main like passes which solely exist to make simulation more convenient.

@sampsyo
Copy link
Contributor

sampsyo commented Jul 24, 2023

This is a nice idea. To expand on this a tiny bit:

  • The way the "simulation" wrapper would have to work is by re-instantiating the memories. That is, the wrapper could consist of Calyx code that literally instantiates, e.g., seq_mem_d1 or whatever. It will also need some Verilog magic to fill those memories in with readmemh and writememh, so maybe it's better if there is a separate library of memory primitives (not seq_mem_*) that behave the same but contain those read/write directives. The wrapper could then pass these memory cells into the "wrapped" program as ref cells.
  • If these wrappers (simulation and AXI) are going to be separate tools, we will face the problem of designing the "language" that describes what the wrapper needs to provide. I'm imagining a simple JSON format, for instance, that just gives the names and types of the memories. I suppose the Calyx compiler would emit this and these external wrapper tools would consume it. Otherwise, the external wrapper tools would need to parse and analyze the Calyx code themselves, which is also a viable option.

@sampsyo
Copy link
Contributor

sampsyo commented Feb 10, 2024

@ayakayorihiro's efforts on a FIRRTL backend for Calyx (cf #1898, most recently) have brought up this issue again, and crystallized a bit more what we need to do here. Specifically, I think this is what we should probably do at some point:

Deprecate @external, replace with ref

The @external annotation originally served two purposes:

  1. In simulation/test mode, emit readmemh/writememh to map memories onto files.
  2. In synthesis mode, turn main "inside out" so these memories are wired up to the outside world instead of being actually present inside main.

Since then, we invented ref, which is a more general solution to problem 2. An elegant way forward here would be to stop using @external altogether and just use ref wherever we would do that. In fact, @ayakayorihiro's FIRRTL backend is already doing exactly this and it seems to work out.

Fancy ref-based testbench

Of course, to address problem 1 above using ref, we need to do what @rachitnigam proposes above:

We change the test harness to somehow read and write in values for all the memories manually

and:

We have another tool, kept outside the compiler source, that wraps things nicely with a module capable to generating the current readmemh and writememh code.

@ayakayorihiro has already hacked together a basic version of this that generates a Verilog testbench this way. The interesting work to be done here would be to (a) base this generator on YXI descriptions, and (b) maybe see if we can simplify it by generating Calyx code instead of Verilog code for parts of it. (That is, we would consider generating a new top-level component that does invoke main[...] and passes in a bunch of memories here.) Not sure exactly where the balance will be here between Calyx and Verilog, but the hope would be to keep as much as possible in Calyx outside of what truly needs to be Verilog (such as the *memh calls themselves).

Deprecate --synthesis

At this point, I think we would no longer need the --synthesis CLI flag to the compiler. Its main purpose is to change how @external works; without that, the code generated for simulation and synthesis could be the same. Which seems like a nice property by itself!

@rachitnigam
Copy link
Contributor Author

Awesome, I support getting rid of all of this cruft (@external and --synthesis)! We should decide if we want to make it a part of this release or not because it's going to make CIRCT stuff break since it does not support ref.

@nathanielnrn
Copy link
Contributor

Just a little something to chime in on, migrating from @external to ref would reduce the amount of boiler plate needed to get our AXI-wrapper working and is currently how our MVP works. That is, our AXI wrapper expects programs to use ref memories, as opposed to @external memories.

Also, one thing to note is that programs using ref will be able to integrates "as is" with the wrapper. While programs using @external will need go through the externalize pass before they can be wrapped. Not a problem in itself but perhaps supports some notion that ref is more flexible out-of-the-box compared to @external.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Simulation Changes for the simulation backend code
Projects
None yet
Development

No branches or pull requests

3 participants