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.

Sign up for GitHub

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

rtlil: Speeds up string decoding by 30% #3940

Closed
wants to merge 1 commit into from
Closed

rtlil: Speeds up string decoding by 30% #3940

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 49 additions & 25 deletions kernel/rtlil.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include <string.h>
#include <algorithm>
#include <array>

YOSYS_NAMESPACE_BEGIN

Expand Down Expand Up @@ -200,17 +201,30 @@ const pool<IdString> &RTLIL::builtin_ff_cell_types() {
return res;
}

RTLIL::Const::Const(const std::string &str)
{
flags = RTLIL::CONST_FLAG_STRING;
bits.reserve(str.size() * 8);
for (int i = str.size()-1; i >= 0; i--) {
unsigned char ch = str[i];
for (int j = 0; j < 8; j++) {
bits.push_back((ch & 1) != 0 ? State::S1 : State::S0);
ch = ch >> 1;
}
}
PRIVATE_NAMESPACE_BEGIN

using CharToBitsLUT = std::array<std::array<RTLIL::State, 8>, 256>;

const CharToBitsLUT build_CharToBitsLUT() {
CharToBitsLUT LUT;
for (int i = 0; i < 256; ++i) {
unsigned char ch = i;
for (int j = 0; j < 8; j++) {
LUT[i][j] = (ch & 1) != 0 ? State::S1 : State::S0;
ch = ch >> 1;
}
}
return LUT;
}
PRIVATE_NAMESPACE_END

RTLIL::Const::Const(const std::string &str) : flags(RTLIL::CONST_FLAG_STRING) {
static const CharToBitsLUT LUT = build_CharToBitsLUT();
bits.reserve(str.size() * 8);
for (int i = str.size() - 1; i >= 0; --i) {
const unsigned char ch = str[i];
bits.insert(bits.end(), LUT[ch].begin(), LUT[ch].end());
Copy link
Member

Choose a reason for hiding this comment

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

I am bit surprised by this being faster -- would you perhaps have a figure for this change alone?

Copy link
Contributor

Choose a reason for hiding this comment

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

This in itself is about 20% faster. Decode_string is about 30% faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before:
image

After:
image

}
}

RTLIL::Const::Const(int val, int width)
Expand Down Expand Up @@ -311,20 +325,30 @@ RTLIL::Const RTLIL::Const::from_string(const std::string &str)
return c;
}

std::string RTLIL::Const::decode_string() const
{
std::string string;
string.reserve(GetSize(bits)/8);
for (int i = 0; i < GetSize(bits); i += 8) {
char ch = 0;
for (int j = 0; j < 8 && i + j < int (bits.size()); j++)
if (bits[i + j] == RTLIL::State::S1)
ch |= 1 << j;
if (ch != 0)
string.append({ch});
}
std::reverse(string.begin(), string.end());
return string;
std::string RTLIL::Const::decode_string() const {
const int n = GetSize(bits);
const int n_over_8 = n / 8;
std::string s;
s.reserve(n_over_8);

auto fill_byte = [&](int start, int end) {
char ch = 0;
for (int j = start; j < end; j++) {
if (bits[j] == RTLIL::State::S1) {
ch |= 1 << (j - start);
}
}
if (ch != 0) s.append({ch});
};

int i = n_over_8 * 8;
if (i < n) {
fill_byte(i, n);
Copy link
Member

@povik povik Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I must say I really dislike the closure here. What about something like:

    int cap = GetSize(bits); // where the current byte ends
    for (int i = GetSize(bits) / 8 * 8; i >= 0; i -= 8) {
        for (int j = i; j < cap; j++) {
            ...
        }
        cap = i;
    }

Copy link
Contributor

@rmlarsen rmlarsen Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Having a named helper function for filling each byte is more readable to me, but I suppose it's a matter of what style you are used to reading. But we can certainly change it.

Copy link
Member

Choose a reason for hiding this comment

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

Please do change it. While at it, please fix the patch to use tabs like in the rest of the file.

Copy link
Member

Choose a reason for hiding this comment

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

Also to consider is splitting the patches: we are ready to merge the decode_string() change but we are not so sure about the lookup table in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

@QuantamHD do you want to update / split the PR or should I just file a new one?

}
for (; i >= 8; i -= 8) {
fill_byte(i-8, i);
}
return s;
}

bool RTLIL::Const::is_fully_zero() const
Expand Down
Loading