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

Latest version of SAW runs out of memory when loading JSON files that previously succeeded #1944

Closed
fandersengalois opened this issue Sep 20, 2023 · 5 comments · Fixed by #2131
Assignees

Comments

@fandersengalois
Copy link

fandersengalois commented Sep 20, 2023

When using the version pointed to in my environment as saw.brett saw runs out of memory when loading a json file:

fandersen@000477fandersen Brett % saw.brett
saw.brett
 ┏━━━┓━━━┓━┓━┓━┓
 ┃ ━━┓ ╻ ┃ ┃ ┃ ┃
 ┣━━ ┃ ╻ ┃┓ ╻ ┏┛
 ┗━━━┛━┛━┛┗━┛━┛ version 1.0.0.99 (cca5d535c)
sawscript> enable_experimental
enable_experimental
sawscript> mcr <- yosys_import "encrypt256_clocked.json";
mcr <- yosys_import "encrypt256_clocked.json";
zsh: killed     saw.brett

The previous SAW version (0.9.0.99 (1d1e1ed5) worked.
These are the vesions used for the testing:

fandersen@000477fandersen Brett % saw.good --version
saw.good --version
0.9.0.99 (1d1e1ed5)
fandersen@000477fandersen Brett % saw.brett --version
saw.brett --version
1.0.0.99 (cca5d535c)

encrypt256_clocked.txt

The JSON file (encrypt256_clocked.json) that fails to load is attached.
Notice that I had to rename encrypt256_clocked.json to encrypt256_clocked.txt to be able to attach it.

@RyanGlScott
Copy link
Contributor

For reference, the good commit (1d1e1ed) is on a SAW branch that was never merged into master, but a slightly tweaked version of that branch was later merged into SAW in 8eaf10e. I've verified that the yosys_import command above works successfully on 8eaf10e (in about 66 seconds on my machine).

@RyanGlScott
Copy link
Contributor

I've confirmed that the regression was introduced in #1880. Specifically, either commit 509855d or d8a5fc0 (it's hard to know for sure, as 509855d doesn't build).

@bboston7 bboston7 self-assigned this Oct 4, 2023
@podhrmic
Copy link

@RyanGlScott what is the status of this issue? @staslyakhov reported that the bug is still present.

mccleeary-galois added a commit that referenced this issue Oct 15, 2024
Delete expiremental CompositionalTranslation and convert back to using Netgraph in Yosys as the CompositionalTranslation graph was ballooning in memory.
mccleeary-galois added a commit that referenced this issue Oct 15, 2024
Delete expiremental CompositionalTranslation and convert back to using Netgraph in Yosys as the CompositionalTranslation graph was ballooning in memory.
@RyanGlScott
Copy link
Contributor

No one has worked on this issue as far as I am aware, so the bug remains. My understanding is that:

  1. SAW's current approach to compositional VHDL verification is experimental, and its performance leaves a lot to be desired.
  2. Therefore, using the compositional verification approach by default is somewhat questionable.

As such, we should reconsider whether we should include the current approach to compositional VHDL verification at all in SAW. In fact, #2131 proposes to remove it, instead using the old, non-compositional approach to VHDL verification.

mccleeary-galois added a commit that referenced this issue Oct 16, 2024
Delete expiremental CompositionalTranslation and convert back to using Netgraph in Yosys as the CompositionalTranslation graph was ballooning in memory.
mccleeary-galois added a commit that referenced this issue Oct 16, 2024
@sauclovian-g
Copy link
Contributor

(syncing metadata with reality)

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 a pull request may close this issue.

6 participants