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

Represent string constants as strings #4516

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

widlarizer
Copy link
Collaborator

@widlarizer widlarizer commented Jul 31, 2024

Currently, a RTLIL::Const representing a string will use std::vector<RTLIL::State>, encoding the string into a vector of individual bits. This increases the memory consumption of string attributes, such as src and hdlname, by a factor of 8.

As of this PR, the structure now holds data represented with an std::variant<bitvectype, std::string> named backing a union. The bits vector is no longer accessible and instead std::vector<RTLIL::State>& bits() has to be used. When a Const is constructed from a string, it is represented with the string alternative. When its bits() method is used, the string is encoded into the bit vector alternative as prior to this PR, see Const::bitvectorize(). This is similar to how RTLIL::SigSpec "unpacks" into bits.

In the initial implementation, when bits() were being accessed, I asserted that the Const isn't represented with a string. This found some places in Yosys that access string attributes bit-by-bit. As a result, this PR comes with bonus content:

  • kernel/cellaigs.cc
  • frontends/ast/ast.cc
  • Const::pretty_fmt
  • cell checker is_param that doesn't try return bits

It's possible to split these now necessary changes out to a separate PR, but it will be heavy on git conflicts. Please advise.

This PR does not affect constant SigChunks which also have an std::vector<RTLIL::State> each.

  • clean my desk (TODO, log_debug, did_something_hook, unwrap std::get from assert_get_bits etc)
  • add some way of debugging how much unpacking we're doing requires unrelated changes in followup PR

Please comment what memory usage changes you see with this PR. I've observed memory savings of 0-20% depending on the design with negligible runtime improvements. The open source Verilog frontend often dominates memory usage, so flows that use Verific may more readily show these improvements.

kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch from 60fa41a to 04d9ec6 Compare August 2, 2024 20:54
@povik
Copy link
Member

povik commented Aug 5, 2024

Please comment what memory usage changes you see with this PR.

On the sky130hd/jpeg design from OpenROAD-flow-scripts, dumping the netlist just after the techmap command, I see the following difference in memory usage when loading in the RTLIL dump in a fresh Yosys process:

with src attributes src stripped
baseline 291 MB 180 MB
9aa6a6b 221 MB 172 MB
9aa6a6b + hashlib patch 202 MB 175 MB

@povik
Copy link
Member

povik commented Aug 5, 2024

I have tried the following patch to hashlib on top of the PR, and updated the table:

diff --git a/kernel/hashlib.h b/kernel/hashlib.h
index 6b880f3a6..0e839e73e 100644
--- a/kernel/hashlib.h
+++ b/kernel/hashlib.h
@@ -190,7 +190,7 @@ inline int hashtable_size(int min_size)
 {
        // Primes as generated by https://oeis.org/A175953
        static std::vector<int> zero_and_some_primes = {
-               0, 23, 29, 37, 47, 59, 79, 101, 127, 163, 211, 269, 337, 431, 541, 677,
+               0, 3, 23, 29, 37, 47, 59, 79, 101, 127, 163, 211, 269, 337, 431, 541, 677,
                853, 1069, 1361, 1709, 2137, 2677, 3347, 4201, 5261, 6577, 8231, 10289,
                12889, 16127, 20161, 25219, 31531, 39419, 49277, 61603, 77017, 96281,
                120371, 150473, 188107, 235159, 293957, 367453, 459317, 574157, 717697,

@widlarizer
Copy link
Collaborator Author

Switching from std::variant to a union yielded further 1% runtime and 0-1% memory usage reduction. std::variant always uses a size_t for its tag. This inflated the data structure beyond 32B. Effects of such sizes may become more severe when we play around to custom allocation strategies. I added private accessor methods to give it a safer interface for methods that don't strictly need direct access. I still use direct union and tag access in constructors, the assignment operator, and bitvectorize.

@widlarizer
Copy link
Collaborator Author

@povik the prime list change adds runtime improvements up to 3% on top of this with a 0.5% regression in one case

@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch from 3214b7b to caf5086 Compare August 15, 2024 08:27
@widlarizer widlarizer marked this pull request as ready for review August 15, 2024 09:08
@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch 2 times, most recently from 9a0edd4 to c34691b Compare August 19, 2024 12:30
@povik povik force-pushed the emil/src-attribute-std-string-wip branch from c34691b to f1951b9 Compare August 23, 2024 09:39
frontends/verific/verific.cc Outdated Show resolved Hide resolved
frontends/ast/ast.cc Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
kernel/rtlil.h Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
passes/opt/opt_merge.cc Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
@povik povik force-pushed the emil/src-attribute-std-string-wip branch from dad4b1b to d4a6563 Compare September 2, 2024 12:20
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

Comparison and indexing isn't consistent across backings

kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.h Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
kernel/rtlil.h Outdated Show resolved Hide resolved
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

Looks good apart from the extra cxxrtl change and the extra assert

backends/cxxrtl/runtime/cxxrtl/cxxrtl.h Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
kernel/rtlil.cc Outdated Show resolved Hide resolved
@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch from 767da07 to 005a6fe Compare September 30, 2024 14:36
@widlarizer widlarizer added the merge-after-release Merge: PR should not be included in the next release label Oct 1, 2024
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

I haven't looked at the new unit testing but the Const change itself looks fine with the points from the last review resolved

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Great work!

@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch 2 times, most recently from 0049444 to 76dfda7 Compare October 9, 2024 17:39
@widlarizer
Copy link
Collaborator Author

just some fixes from the functional merges

@widlarizer widlarizer added merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised and removed merge-after-release Merge: PR should not be included in the next release labels Oct 11, 2024
@widlarizer
Copy link
Collaborator Author

I validated the memory savings to be 0-30%.

ORFS peak:

design 5c9b2df main [MB] 2dd0ae3 PR [MB]
asap7_ibex 224.52 194.96
sky130hd_gcd 57.75 57.75
sky130hd_ibex 223.20 159.32
sky130hd_jpeg 726.52 613.00

In the flow I dumped out the RTLIL before synthesis and outside of ORFS loaded the RTLIL and ran just the basic synth command:

design 5c9b2df main [MB] 2dd0ae3 PR [MB]
asap7_ibex 175.88 122.42
sky130hd_gcd 45.56 45.56
sky130hd_ibex 175.76 123.02
sky130hd_jpeg 85.14 75.63

As for runtime, there was negligible improvement (0-1%)

@widlarizer widlarizer force-pushed the emil/src-attribute-std-string-wip branch from 67a97f5 to 2dd0ae3 Compare October 13, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants