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

Entity state saving to binary file #22

Merged
merged 9 commits into from
Nov 4, 2021
Merged

Entity state saving to binary file #22

merged 9 commits into from
Nov 4, 2021

Conversation

ojarvin
Copy link
Contributor

@ojarvin ojarvin commented Oct 25, 2021

Moi,

This branch is supposed to re-factor, clarify and improve the way simulation results and files are stored.
This is for now related to changes in the spice-module, but eventually this state saving/loading should work for any simulator model.

Changes related to file preserving properties:

  • Original version (pre-1.7_RC)
    • preserve_iofiles (some unpredictable behavior)
    • preserve_spicefiles (some unpredictable behavior)
    • load_state (worked fine, loading from iofiles)
  • Previous version (1.7_RC)
    • preserve_iofiles (replaced by load_result)
    • preserve_spicefiles (replaced by load_result)
    • Added load_result to do what old preserve properties did
    • load_state (no changes)
  • This version (save_state)
    • preserve_iofiles (re-added, works as expected)
    • preserve_spicefiles (re-added, works as expected)
    • Added save_state boolean property to enable automatic state saving after simulation (for now only implemented by spice-module)
    • load_state (now loads from a binary entity state file, outside interface unchanged)

These changes are supposed to create clear separation between debug features (preserve_*), which are intended to be False unless some interface debugging is needed, and the result-saving features (save_state & load_state), which are meant for typical testbench usage.

You need to use branch "save_state" in spice if you want to test this branch!

The property simpath is now common to spice and rtl, and it's defined in
thesdk instead of the respective simulator modules.

Tested IO pointers when loading a state from disk, the pointers are now
persistent. Also, added option to load only IOs instead of full state.
@ojarvin
Copy link
Contributor Author

ojarvin commented Oct 28, 2021

Okay I think these changes are done. The overall changes are implemented in thesdk, spice and rtl, where each has a branch save_state. The inverter package has also the same branch which I used to test these changes.

Some things I refactored:

  • spicesimpath and rtlsimpath moved from respective modules under thesdk as simpath (related to Clarify/remove the supported models from thesdk module #12)
  • Default value of simpath is now based on simulator model: the default is now ./simulations/<model>/<runname> as opposed to ./Simulations/<spicesim|rtlsim>/<runname>
  • runname moved from spice/rtl to thesdk

The entity directory structure is now more consistent in my opinion. If this is too large change, please let me know. In terms of simulations and saved states, the directory tree should now look like

inverter
  |- simulations
    |- eldo
      |- run1
      |- run2
    |- spectre
      |- run1
      |- run2
    |- sv
      |- run1
      |- run2
    |- vhdl
      |- run1
      |- run2
  |- states
    |- eldo
      |- run1
      |- run2
    |- spectre
      |- run1
      |- run2
    |- sv
      |- run1
      |- run2
    |- vhdl
      |- run1
      |- run2

For clarity the properties are now

  • save_state: True | False (default). Save entity state to states after simulation
  • load_state: "runname". Load entity state from states instead of running simulation
  • load_state_full: True (default) | False. When load_state is used, load full entity state, otherwise load only IOs and extracts
  • preserve_iofiles: as it was
  • preserve_spicefiles: as it was
  • preserve_rtlfiles: as it was

I confirmed that the IO() object pointers are preserved when the entity state is loaded.

P.S. The output prints look less depressing with the color tags.
P.P.S. I'm not avoiding writing my thesis.

@sporrasm
Copy link
Contributor

My world is now full of colour and joy :)

More seriously, this seems to work well, so green light for merge from my side.

Copy link
Member

@mkosunen mkosunen left a comment

Choose a reason for hiding this comment

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

I agree with the changing Simulations to simulations eventually, but it breaks/may break LOT of references to Simulations directory, so I would leave it as Simulations at this point.

@chiplet
Copy link
Contributor

chiplet commented Nov 3, 2021

The Simulations -> simulations rename should be probably done as a dedicated PR on all affected repos and merged simultaneously after weeding out the all of the out-of-date references.

@mkosunen
Copy link
Member

mkosunen commented Nov 3, 2021

Discussed with @ojarvin , One problematic part of Simulations->simulations is that currently rtl uses dofile.do that is used in interactive modelsim simulations. Eventually that would be the only file left in Simulations that is not created by TheSyDeKick. Way to proceed is as follows.

  1. We create two new attributes, interactive_controlfile, that point the file that can be used as that dofile, and interactive_control_contents that contains the content of that file (it is written in most cases manually anyway).
  2. In rtl package, the run sequence is modified in a way that first a check is made if Simulations/rtlsim/dofile.do exists. if yes, interactive_controlfile is set to point to that, and a obsoletion warning is printed out, giving instructions how to transfer the content to interactive_control_contents.
  3. further operation is as follows: If interactive_control_contents is set,the file is written to a predefined location, and executed as a part of interactive run.

@chiplet Can you see any other things that may break with the proposed change?

I will open another PR for this.
@chiplet
Copy link
Contributor

chiplet commented Nov 3, 2021

This seems like a good approach to me since it doesn't break downstream projects, provides a clear update path, and also increases configurability. The new default for interactive_controlfile should then probably point to simulations/rtlsim/dofile.do (after the check to Simulations) to reduce boilerplate. I can't think of anything else that would break because of this.

@ojarvin
Copy link
Contributor Author

ojarvin commented Nov 3, 2021

Okay the simulation directory structure is now as it was in previous versions. I will probably open a new PR for re-naming it and refactoring the dofile location as well (I see these as somewhat related).

@mkosunen
Copy link
Member

mkosunen commented Nov 3, 2021

Tested with inverter. TO my opinion, can be merged.

@mkosunen
Copy link
Member

mkosunen commented Nov 3, 2021

@ojarvin there seems to be a conflict. WIll you resolve it?

@ojarvin ojarvin merged commit 4d7a087 into v1.7_RC Nov 4, 2021
@ojarvin ojarvin deleted the save_state branch November 4, 2021 08:22
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.

4 participants