rtlil: Speeds up string decoding by 30%

[QuantamHD]

This change represents about a 2% speed up of Yosys as a whole.

Written by @rmlarsen

[rmlarsen]

Did you intend to include the change to decode_string here? Or is that going to be a separate PR?

[QuantamHD]

Good catch. I added it back.

[povik]

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

[rmlarsen]

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

[rmlarsen]

Before:

After:

[povik]

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;
    }

[rmlarsen]

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.

[povik]

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

[povik]

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.

[rmlarsen]

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

[povik]

We are curious about the timings. Couple of questions: when you say this speeds up Yosys as a whole by 2 %, do you mean some representative ASIC flow you are running on your end? Would you have some typical timings for the read_verilog / hierarchy steps on their own, ideally absolute? We assume those steps are where the significant speedup is, e.g. in constructing the Const values for all the src attributes, et cetera.

[rmlarsen]

Hi Martin,

Thanks for the comments. Response below.

We are curious about the timings. Couple of questions: when you say this speeds up Yosys as a whole by 2 %, do you mean some representative ASIC flow you are running on your end? Would you have some typical timings for the read_verilog / hierarchy steps on their own, ideally absolute? We assume those steps are where the significant speedup is, e.g. in constructing the Const values for all the src attributes, et cetera.

Yes, the 2% is the reduction in overall time of the synthesis flow for a representative circuit from a Google hardware block that we are running through Yosys. I can provide you with a flame graph for example, since that probably gives more context, or a wider slice of the Tree view.

[povik]

Hello Rasmus,

Yes, anything you could provide to inform our discussion would be helpful.

[rmlarsen]

@povik Will do. Notice that @QuantamHD is OOO until tomorrow. I'd like to coordinate with him on how to proceed in terms of splitting, so give us a few days. Thanks.

[povik]

We will probably next look at this on Monday 14:00 UTC during the weekly development call (which is btw open to public and you are more than welcome to join if interested).

[rmlarsen]

Ah, good to know. Thanks! Would be nice to join the call if I can at some point.

[povik]

Link here: https://meet.jit.si/yosyshq-slack-devel-discuss

It's scheduled to be an hour long, occurs every week.

[rmlarsen]

@povik I moved the decode_string() change to #3959. I got rid of the lambda and after a few tries found a variant that was a bit faster too.

[QuantamHD]

@rmlarsen Should we close this then?

[rmlarsen]

@QuantamHD sure. We can upstream the Const constructor separately, if the Yosys authors find it acceptable.