Represent string constants as strings #4516

widlarizer · 2024-07-31T14:26:03Z

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

kernel/rtlil.h

kernel/rtlil.cc

backends/cxxrtl/cxxrtl_backend.cc

povik · 2024-08-05T15:48:10Z

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 · 2024-08-05T16:30:50Z

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 · 2024-08-08T09:00:04Z

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 · 2024-08-08T10:34:16Z

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

frontends/verific/verific.cc

povik · 2024-08-23T09:48:27Z

frontends/ast/ast.cc

@@ -1749,16 +1741,7 @@ static std::string serialize_param_value(const RTLIL::Const &val) {
 		res.push_back('r');
 	res += stringf("%d", GetSize(val));
 	res.push_back('\'');
-	for (int i = GetSize(val) - 1; i >= 0; i--) {


This is unrelated to the storage change

I still think refactoring into as_string is the correct thing to do when we have the opportunity

povik · 2024-08-23T09:55:51Z

kernel/rtlil.cc

-			return bits[i] < other.bits[i];
+	const char* ctx = "operator<";
+	if (std::get_if<std::string>(&backing) != std::get_if<std::string>(&other.backing))
+		return decode_string() < other.decode_string();


This doesn't semantically match the comparison over bits (first character of the string is in the highest 8 bits, so the lexicographic ordering is different)

Also this doesn't match if (sizes unequal) return bits.size() < other.bits.size() from the other branch

Should be fixed now

kernel/rtlil.cc

kernel/rtlil.h

kernel/rtlil.cc

passes/opt/opt_merge.cc

kernel/rtlil.h

povik

Comparison and indexing isn't consistent across backings

kernel/rtlil.cc

kernel/rtlil.h

kernel/rtlil.cc

kernel/rtlil.h