Skip to content

Conversation

@rocallahan
Copy link
Contributor

If your work is part of a larger effort, please discuss your general plans on Discourse first to align your vision with maintainers.

See https://yosyshq.discourse.group/t/parallel-optmergepass-implementation/87/10.

What are the reasons/motivation for this change?

SigSpec::operator[] const currently switches the SigSpec to the unpacked representation. const methods that mutate the data make the type unsuitable for read-only sharing across threads, so we'd like to eliminate this. This PR is the start of that work.

Explain how this is achieved.

We make SigSpecConstIterator work efficiently with both packed and unpacked SigSpecs. Then we migrate some common in-tree users of SigSpec::operator[] const to use iterators instead.

In future work we will migrate more in-tree users. For the remaining users we will investigate some kind of caching scheme to make SigSpec::operator[] const reasonably efficient without switching to the unpacked representation.

@jix jix self-assigned this Oct 9, 2025
Copy link
Collaborator

@widlarizer widlarizer left a comment

Choose a reason for hiding this comment

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

The code looks fine and matches what was set out to do, but similarly to #5417 (review) unfortunately there's an overall regresion to jpeg synthesis, but specifically opt_clean regresses by 10% with smaller but still significant hits to opt_expr and opt_dff. The change isn't performance transparent. Again this is with LTO on. The opt_expr and opt_dff regressions are observable on eff68a4 already, which surprises me, since I would have thought that SigSpec::find_chunk is where extra runtime would be coming from

@rocallahan
Copy link
Contributor Author

rocallahan commented Oct 20, 2025

I see most the regression happening in the very first commit, which is weird:

main:
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; synth"
  Time (mean ± σ):      5.950 s ±  0.004 s    [User: 5.604 s, System: 0.351 s]
  Range (min … max):    5.944 s …  5.956 s    10 runs

Avoid unnecessary copying when applying a SigMap to a SigSpec
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; synth"
  Time (mean ± σ):      6.034 s ±  0.013 s    [User: 5.700 s, System: 0.340 s]
  Range (min … max):    6.013 s …  6.055 s    10 runs
 
sigspec-const:
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; synth"
  Time (mean ± σ):      6.050 s ±  0.009 s    [User: 5.695 s, System: 0.361 s]
  Range (min … max):    6.037 s …  6.068 s    10 runs

I think the problem is that that commit switches the passed-in representation of some SigSpec's to unpacked, which then has to be switched back later, whereas the current code would instead copy the packed SigSpec, unpack it, and then throw away the unpacked verison

Another difference is that the new code builds a packed SigSpec result, which then has to be unpacked by some callers, whereas the current code always returns an unpacked SigSpec. Maybe that's part of the problem too.

@rocallahan rocallahan marked this pull request as draft October 21, 2025 21:30
@rocallahan
Copy link
Contributor Author

Taking this off the review queue while I study the problem and investigate alternatives.

To make this efficient, the iterator keeps `chunk_index_hint` pointing to
the current chunk. This is only a hint, since it is not possible to maintain
its validity if the `SigSpec` representation is temporarily changed to unpacked.
In that case we have to resync using binary search.
Currently we duplicate the `SigSpec` first and then iterate over the
duplicate, updating its bits. Instead just generate new bits and add
them to an empty `SigSpec`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants