-
Notifications
You must be signed in to change notification settings - Fork 399
synth: add SYNTH_MOCK_LARGE_MEMORIES #3621
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
synth: add SYNTH_MOCK_LARGE_MEMORIES #3621
Conversation
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
flow/scripts/synth.tcl
Outdated
| set size [memory_get $module $mem SIZE] | ||
|
|
||
| if {$size > $::env(SYNTH_MEMORY_MAX_BITS)} { | ||
| memory_set $module $mem SIZE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oharboe Is memory_set a hypothetical command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there are other dreamed up commands. Let me see if this can be implemented using real commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In ChatGPT's defense it did say that it dreamt up these commands and then it looked for alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I offer this for testing:
memory_collect
foreach path [tee -q -s result.string select -list t:\$mem_v2] {
set index [string first "/" $path]
set module [string range $path 0 [expr {$index - 1}]]
set instance [string range $path [expr {$index + 1}] end]
set width [rtlil::get_param -uint $module $instance WIDTH]
set size [rtlil::get_param -uint $module $instance SIZE]
set nbits [expr $width * $size]
puts "Memory $path has dimensions $size x $width = $nbits"
if {$nbits > $::env(SYNTH_MEMORY_MAX_BITS)} {
rtlil::set_param -uint $module $instance SIZE 1
puts "Shrunk memory $path from $size rows to 1"
}
}
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@povik Mysterious error: |
|
I've found the message template in Yosys. It warns about a special character but at the same time it prints the string containing the special character without escaping hence the mangled output.
I'll take a look to see how to fix the script. |
|
@povik Purely for my planning purposes, how long do you think you'll need to see if there's a fix for this PR? Days, weeks, months? If it is very far out, then I think I should close the issue and re-open when we have what we need. |
|
@oharboe a day or two |
|
@oharboe needs the fix below (can't push to your branch): |
Signed-off-by: Martin Povišer <povik@cutebit.org>
|
@maliberty Thoughts? |
|
@jeffng-or Thoughts? |
| Memories with a single 1 row will of course have unrealistically good | ||
| timing and area characteristics, but timing will still correctly terminate | ||
| in a register. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me also point out a concern that some of the logic driving the address input on the RAM will be optimized out when we override the RAM size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I think these sort of downstream consequences are evident to anyone skilled in the art given the warning in the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. In my experience people are surprised by what can get optimized out when they mock something in their design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.... The idea is that whatever problems remain after mocking, they are real place/route/timing closure problems and worth sorting out while waiting for real RAM or some more accurate fake RAM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a more stern comment to the effect "you asked for it and you deserve the result" would be helpful, but I think it is fine the way it is.
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
SYNTH_MOCK_LARGE_MEMORIES only work for memories that are inferred by Yosys, the empty RAM512.v is not a memory that is inferred by Yosys. So neither fakeram nor these these gutted RAMs that I've seen in a couple of places are inferred memories by Yosys and therefore not affected by SYNTH_MOCK_LARGE_MEMORIES. Some working notes before we talk Monday. The modules above with inferred memories are now kept by default:
|
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
Tried this on a larger design that I can't share, worked very well. I was able to get Yosys to assert for a 2048+ bits wide 2 row deep memory, but by tweaking configuration settings, I made it past that snag. |
|
@povik The assert isn't holding me up, but I've left confidential instructions with @maliberty on how to reproduce it in case there's a need to fix it. |
|
Testing CoralNPU: Now do a quick build to see what we have... This lists memories that are too large: (running) @povik Hmmm... I wonder if slang has difficulty inferring memories. The above doesn't infer a memory. |
|
Yosys slang may be having trouble inferring memories. untar yosys-no-memories.tar.gz Run: I expected this error to kick in: OpenROAD-flow-scripts/flow/scripts/synth.tcl Lines 59 to 61 in 2b52c77
But it doesn't... |
|
@povik FYI, unrelated to this PR as such, but when using Untar slang-no-infer-mem.tar.gz Run the below Fails quickly, as expected, memories are inferred: Rerun the above, but with
|
|
@oharboe I now recall we use the We might be able to drop the flag. It was a workaround for an issue which may have been fixed since. |
|
The flag is used with read_slang in synth_preamble.tcl |
|
Recording notes while I kick off a larger run... diff --git a/flow/scripts/synth_preamble.tcl b/flow/scripts/synth_preamble.tcl
index a37231275..c20bb2836 100644
--- a/flow/scripts/synth_preamble.tcl
+++ b/flow/scripts/synth_preamble.tcl
@@ -47,7 +47,7 @@ proc read_design_sources { } {
# slang requires all files at once
plugin -i slang
yosys read_slang -D SYNTHESIS --keep-hierarchy --compat=vcs \
- --ignore-assertions --no-implicit-memories --top $::env(DESIGN_NAME) \
+ --ignore-assertions --top $::env(DESIGN_NAME) \
{*}$vIdirsArgs {*}$::env(VERILOG_FILES) {*}[env_var_or_empty VERILOG_DEFINES]
# Workaround for yosys-slang#119
setattr -unset initAfter 15 minutes: Next run to see if we can get this past synthesis so we can check the sizes of the various modules in gui_synth: |
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| @@ -0,0 +1,26 @@ | |||
| # Tips on building large design | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@povik Any thoughts on synthesis optimizations that I can turn off to speed up builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance synthesis is gnawing on some bone here "forever":
8.4.544. Executing OPT_EXPR pass (perform const folding).
8.4.545. Executing OPT_MUXTREE pass (detect dead branches in mux trees).
8.4.546. Executing OPT_EXPR pass (perform const folding).
8.4.547. Executing OPT_MUXTREE pass (detect dead branches in mux trees).
8.4.548. Executing OPT_EXPR pass (perform const folding).
8.4.549. Executing OPT_MUXTREE pass (detect dead branches in mux trees).
8.4.550. Executing OPT_EXPR pass (perform const folding).
8.4.551. Executing OPT_MUXTREE pass (detect dead branches in mux trees).
8.4.552. Executing OPT_EXPR pass (perform const folding).
8.4.553. Executing OPT_MUXTREE pass (detect dead branches in mux trees).
8.4.554. Executing OPT_EXPR pass (perform const folding).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen people disable the share pass as runtime intensive (option synth -noshare). In your case it's stuck elsewhere and I don't have a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to generate a report with sizes for the modules before the build starts?
That could clue me in to what is going on, much like the max memory bits early failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can run stat at the end of the canonicalize step. The printed statistics would be in terms of internal yosys cells but that might be useful too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get stat per module and create a table in github .md syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not out of the box but with an investment we could
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed a feature request: #3645
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what the canonicalize stat output looks like on the jpeg design: https://gist.github.com/povik/deb2a3c32ef989a5371ce606516661c1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it contains all the information, but the formatting is terrible.
If this was in a table tailored to the use-case of finding pathologically large modules, I think we'd have something.
|
This PR is good to go for my part now. Things are working pretty well. When I push yosys/synthesis hard enough, I, no surprise, find that there is always room for improvement, but that's beyond this PR. |
docs/user/FlowVariables.md
Outdated
| | <a name="SYNTH_HDL_FRONTEND"></a>SYNTH_HDL_FRONTEND| Select an alternative language frontend to ingest the design. Available option is "slang". If the variable is empty, design is read with the Yosys read_verilog command.| | | ||
| | <a name="SYNTH_HIERARCHICAL"></a>SYNTH_HIERARCHICAL| Enable to Synthesis hierarchically, otherwise considered flat synthesis.| 0| | ||
| | <a name="SYNTH_HIER_SEPARATOR"></a>SYNTH_HIER_SEPARATOR| Separator used for the synthesis flatten stage.| .| | ||
| | <a name="SYNTH_KEEP_MOCKED_MEMORIES"></a>SYNTH_KEEP_MOCKED_MEMORIES| Keeping the mocked memories(not flattening them), preserves some of the access logic complexity and avoids optimizations outside of the mocked memory.| 1| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that this only has an effect if SYNTH_MOCK_LARGE_MEMORIES is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

@povik Is there a reasonable way to implement such a SRAM mocking feature?