-
Notifications
You must be signed in to change notification settings - Fork 872
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
base: main
Are you sure you want to change the base?
Conversation
60fa41a
to
04d9ec6
Compare
On the sky130hd/jpeg design from OpenROAD-flow-scripts, dumping the netlist just after the
|
I have tried the following patch to hashlib on top of the PR, and updated the table:
|
Switching from |
@povik the prime list change adds runtime improvements up to 3% on top of this with a 0.5% regression in one case |
3214b7b
to
caf5086
Compare
9a0edd4
to
c34691b
Compare
c34691b
to
f1951b9
Compare
@@ -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--) { |
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.
This is unrelated to the storage change
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 still think refactoring into as_string is the correct thing to do when we have the opportunity
kernel/rtlil.cc
Outdated
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(); |
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.
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)
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.
Also this doesn't match if (sizes unequal) return bits.size() < other.bits.size()
from the other branch
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.
Should be fixed now
dad4b1b
to
d4a6563
Compare
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.
Comparison and indexing isn't consistent across backings
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 good apart from the extra cxxrtl change and the extra assert
@@ -750,7 +750,7 @@ std::ostream &operator<<(std::ostream &os, const value<Bits> &val) { | |||
auto old_flags = os.flags(std::ios::right); | |||
auto old_width = os.width(0); | |||
auto old_fill = os.fill('0'); | |||
os << val.bits << '\'' << std::hex; | |||
os << val << '\'' << std::hex; |
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.
val
is not a Const, we shoudn't be touching this
if ((*this)[i] != other[i]) | ||
return (*this)[i] < other[i]; | ||
|
||
return size() < other.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.
This changes the semantics of comparison. I'm not sure if we rely on it anywhere
|
||
std::vector<RTLIL::State>& RTLIL::Const::bits() | ||
{ | ||
log_assert(is_bits()); |
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.
Why this assert? Maybe a leftover from experiments
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.
It's redundant now, but it wasn't before
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.
As I understand it would crash on the following
Const c("blah"s);
c.bits();
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.
true, I misread the called code
@@ -3730,7 +3913,7 @@ RTLIL::SigChunk::SigChunk(const RTLIL::SigBit &bit) | |||
wire = bit.wire; | |||
offset = 0; | |||
if (wire == NULL) | |||
data = RTLIL::Const(bit.data).bits; | |||
data = RTLIL::Const(bit.data).to_bits(); |
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.
Uh, this was here before the PR, but why does this go through a Const
?
We could do data = {bit.data};
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.
nice catch
Currently, a
RTLIL::Const
representing a string will usestd::vector<RTLIL::State>
, encoding the string into a vector of individual bits. This increases the memory consumption of string attributes, such assrc
andhdlname
, by a factor of 8.As of this PR, the structure now holds data represented with
ana union. Thestd::variant<bitvectype, std::string>
namedbacking
bits
vector is no longer accessible and insteadstd::vector<RTLIL::State>& bits()
has to be used. When a Const is constructed from a string, it is represented with the string alternative. When itsbits()
method is used, the string is encoded into the bit vector alternative as prior to this PR, seeConst::bitvectorize()
. This is similar to howRTLIL::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
is_param
that doesn't try return bitsIt'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
SigChunk
s which also have anstd::vector<RTLIL::State>
each.std::get
fromassert_get_bits
etc)add some way of debugging how much unpacking we're doingrequires unrelated changes in followup PRPlease 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.