-
Notifications
You must be signed in to change notification settings - Fork 998
Make SigSpec const methods multithreading-compatible
#5453
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
Open
rocallahan
wants to merge
30
commits into
YosysHQ:main
Choose a base branch
from
rocallahan:sigspec-onechunk
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+669
−551
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hout packing the SigSpec
…equality, just use the width and chunkwise comparisons This avoids having to pack and compute hashes, and generally results in a simpler ordering.
…bits are modified
… that's inline in the SigSpec. Single-chunk SigSpecs are very common and this avoids a heap allocation. It also simplifies some algorithms.
76246c9 to
0d3cd5d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, although I've changed the approach somewhat. #5415 is also relevant, albeit superseded by this PR.
What are the reasons/motivation for this change?
We want to make
SigSpecusable in multithread contexts. This requires making allconstmethods actually read-only.Explain how this is achieved.
The main idea here is to change the representation of
SigSpecfrom "vector ofSigChunkor vector ofSigBit" to "single inlinedSigChunkor vector ofSigBit". That change makes it quite easy to implementSigSpec::operator[] constefficiently in a read-only manner.Making that change requires updating all code that directly accesses the
chunks_vector to use a "chunks iterator" which can work with either representation.The new representation works well in practice. I did some analysis of the use of
SigSpecs and observed that mostSigSpecscan be representated as a singleSigChunk(partly because mostSigSpecs are actually a single bit). I actually see a nice speedup from this PR:The speedup probably comes partly from avoiding heap allocation in the single-
SigChunkcase, and also from the fact that we avoid repeated representation flipping. In the new code,SigSpecs (if not empty) start off with theSigChunkrepresentation, and we keep them in that representation as long as possible. If at some point we need to unpack to the bits representation, we do that and then it uses the bits representation forever.Of course it is still possible to construct testcases where this PR would regress performance --- for example if you have a lot of
SigSpecs which can be compactly represented as two very wideSigChunks. I hope that kind of thing is not a problem in practice but you never know... If you have any testcases you want me to investigate I'd be happy to do that.You can see from the commits that I was able to maintain source compatibility with the rest of the in-tree code in almost all cases. The main issue that worries me is that a few callers to
SigSpec::chunks()assume it returns the same underlying object in all cases so e.g. an iterator returned bysig.chunks().begin()can always be compared tosig.chunks().end().I'm sorry that this PR is quite large, but to avoid regressing performance we need to merge a lot of work at once. The final commit fixes the thread-safety of
hash_and is probably performance-neutral, so could be broken out if desired.