Improve opt_merge performance

[povik]

See the commits. This replaces the hashing implementation, and makes it ignore cells holding memory initialization data.

Ping @rmlarsen since they were interested in opt_merge performance before.

[rmlarsen]

Cool. I'll run our benchmarks on it when I get a chance.

[rmlarsen]

For our circuit, this reduces the time spent in OptMerge from ~25.5s to ~21.8s, so a ~17% speedup.

Flame graph before:

Flame graph after:

[povik]

@rmlarsen Interesting. There seem to be much more time spent in compare_cell_parameters_and_connections after the change, so that suggests there's more false positives where the hash matches but the cells actually don't.

[rmlarsen]

@povik yeah, that is odd. Is the mkhash function not mixing the hashes sufficiently?

[rmlarsen]

@povik yeah mkhash creates a simple 32 bit hash:

// The XOR version of DJB2
inline unsigned int mkhash(unsigned int a, unsigned int b) {
	return ((a << 5) + a) ^ b;
}

You end up combining a lot of those potentially. So the hashing is definitely weaker.

[QuantamHD]

I wonder if pulling in xxhash or cityhash would be an improvement here.

[rmlarsen]

BTW: This code is really weird. unsigned int is 32 bit per the C++ standard.

yosys/kernel/hashlib.h

Line 39 in 1b73b5b

inline unsigned int mkhash_xorshift(unsigned int a) {

[rmlarsen]

I don't quite grok what's going on here, but it seems quite different from the original hashing of connections. Could this be the source of more collisions?

[povik]

BTW: This code is really weird. unsigned int is 32 bit per the C++ standard.

yosys/kernel/hashlib.h

Line 39 in 1b73b5b

inline unsigned int mkhash_xorshift(unsigned int a) {

Right, that doesn't make much sense.

[povik]

Turns out the collisions are due to a combination of mkhash being a weak mixing primitive, and Const hashing not working at all (hash() for those returns a constant due to a coding error). Let me collect my changes and push.

[nakengelhardt]

BTW: This code is really weird. unsigned int is 32 bit per the C++ standard.

Wait, what? ILP64 may not be popular but since when does the C++ standard ban it?

[povik]

BTW: This code is really weird. unsigned int is 32 bit per the C++ standard.

Wait, what? ILP64 may not be popular but since when does the C++ standard ban it?

Second closer look at https://en.cppreference.com/w/cpp/language/types indeed confirms the standard allows int be 64-bit, the page just doesn't list it as an option in the "common data models" table. I will revert the mkhash_xorshift change.

[povik]

I will revert the mkhash_xorshift change.

On second thought those ILP64 being so rare there's no point keeping a special branch for them, if the default branch is basically fine too. I think I will keep the change in.

[nakengelhardt]

It's currently the only way to get around the limitation on design size, which people do run into somewhat regularly.

yosys/kernel/hashlib.h

Lines 203 to 204 in d00843d

	if (sizeof(int) == 4)
	throw std::length_error("hash table exceeded maximum size.\nDesign is likely too large for yosys to handle, if possible try not to flatten the design.");

I don't know if anyone has taken this path in practice, but from past conversations in issues I suspect some people patched yosys locally to replace "int" with "long". We might want to go that way long-term too... we've stayed away from it so far out of fear of introducing more subtle issues that our tests won't be able to uncover, but given how we keep finding that the hash wasn't working properly in the first place maybe we should just go for it.

Either way we should probably take that discussion to a separate issue/PR.

[rmlarsen]

@nakengelhardt @povik if this is indeed something people struggle with in the Yosys, I'd highly recommend going with the standard types int32_t and int64_t etc. We don't have to suffer these indignities anymore.

[nakengelhardt]

@nakengelhardt @povik if this is indeed something people struggle with in the Yosys, I'd highly recommend going with the standard types int32_t and int64_t etc. We don't have to suffer these indignities anymore.

How does that help, what benefit do you get from requiring a precise size rather than a minimum size here?

[rmlarsen]

@nakengelhardt @povik if this is indeed something people struggle with in the Yosys, I'd highly recommend going with the standard types int32_t and int64_t etc. We don't have to suffer these indignities anymore.

How does that help, what benefit do you get from requiring a precise size rather than a minimum size here?

Well defined semantics of your code, perhaps?

[povik]

But it doesn't seem like the lack of defined semantics is the issue here, or something anyone is struggling with. It seems like the underspecification of int is a feature right now, there's sizeof(int) == 4 which is the well-tested case, and sizeof(int) == 8 for adventurers who are hitting limits otherwise. Of course eventually moving to X with fixed sizeof(X) == 8 in the code where people are hitting limits is desirable.

[rmlarsen]

But it doesn't seem like the lack of defined semantics is the issue here, or something anyone is struggling with. It seems like the underspecification of int is a feature right now, there's sizeof(int) == 4 which is the well-tested case, and sizeof(int) == 8 for adventurers who are hitting limits otherwise. Of course eventually moving to X with fixed sizeof(X) == 8 in the code where people are hitting limits is desirable.

Sure. But in this instance, the code is less readable and has different behavior on different platforms, which should be avoided unless absolutely necessary IMHO.

[nakengelhardt]

Yes, we do need configurability of the hash size for different use cases. The current way of doing it has just fallen out of favor since (or possibly before) this code was first written. Ideally we would have 64b hash be the default on 64b architectures, but we need to maintain the option of 32b hashes for smaller/slower architecture targets where the performance hit matters (people running yosys on an older raspi are still a thing, and then there's that proof-of-concept someone made of a softcore doing synthesis to partially reconfigure the FPGA it is running on, which we want to enable just for the heck of it). So there's no way around it that any caller of the function has to work for all sizes anyway.

[povik]

s/int/long/ in the hashing code then? 😄

[rmlarsen]

Yes, we do need configurability of the hash size for different use cases. The current way of doing it has just fallen out of favor since (or possibly before) this code was first written. Ideally we would have 64b hash be the default on 64b architectures, but we need to maintain the option of 32b hashes for smaller/slower architecture targets where the performance hit matters (people running yosys on an older raspi are still a thing, and then there's that proof-of-concept someone made of a softcore doing synthesis to partially reconfigure the FPGA it is running on, which we want to enable just for the heck of it). So there's no way around it that any caller of the function has to work for all sizes anyway.

I see. [This is C++, so this is usually done with explicit overloads, not by adding a runtime check for sizeof. But there appears to be a lot of C code in Yosys.]

[rmlarsen]

s/int/long/ in the hashing code then? 😄

Noooooooo! ;-)

[QuantamHD]

What's the purpose of this custom hash function in the first place? It seems like adopting an existing function that works on char* streams would both be platform independent, and faster than what we've done here.

[rmlarsen]

What's the purpose of this custom hash function in the first place? It seems like adopting an existing function that works on char* streams would both be platform independent, and faster than what we've done here.

Moreover, in the code in question, it is also being used to combine hashes for which this hash function is very poor. There should probably be a separate hash function for that purpose. IFAICT, DJB2 is a very simple hashing for strings.

[rmlarsen]

good catch!

[QuantamHD]

For reference on the state of the art.

[povik]

DJB2 didn't make the leaderboard? Notwithstanding we are probably using it wrong in the changed opt_merge code and in few other places in hashlib.h.

[whitequark]

Ideally we would have 64b hash be the default on 64b architectures, but we need to maintain the option of 32b hashes for smaller/slower architecture targets where the performance hit matters (people running yosys on an older raspi are still a thing, and then there's that proof-of-concept someone made of a softcore doing synthesis to partially reconfigure the FPGA it is running on, which we want to enable just for the heck of it).

Has anyone actually benchmarked this and found that the hash function is an issue? Or is this conjecture?

[rmlarsen]

Ideally we would have 64b hash be the default on 64b architectures, but we need to maintain the option of 32b hashes for smaller/slower architecture targets where the performance hit matters (people running yosys on an older raspi are still a thing, and then there's that proof-of-concept someone made of a softcore doing synthesis to partially reconfigure the FPGA it is running on, which we want to enable just for the heck of it).

Has anyone actually benchmarked this and found that the hash function is an issue? Or is this conjecture?

It is not a hotspot in my benchmarks, but this was measured on Intel Skylake x86_64. The main issue in this context was the poor hashing, which caused a significant number of hash collisions. But I think Martin fixed the primary source of that (lack of hashing of constants).

[povik]

I assume Catherine's question was about hashing in general all over Yosys code, and the associated dict<> and pool<> operations which consume the hashes. Was that what you looked at, Rasmus, or did you mean opt_merge specifically?

But I think Martin fixed the primary source of that (lack of hashing of constants).

FWIW the poor properties of mkhash contributed the most in my testing, which went away when I inserted mkhash_xorshift64 in few key places in the PR here. It may not be the best approach but it's at least something we can improve on.

[rmlarsen]

I assume Catherine's question was about hashing in general all over Yosys code, and the associated dict<> and pool<> operations which consume the hashes. Was that what you looked at, Rasmus, or did you mean opt_merge specifically?

But I think Martin fixed the primary source of that (lack of hashing of constants).

FWIW the poor properties of mkhash contributed the most in my testing, which went away when I inserted mkhash_xorshift64 in few key places in the PR here. It may not be the best approach but it's at least something we can improve on.

Ah yes. Hash table manipulation in general (but not mkhash) still dominates the profile. Top of "Bottom up view":

FWIW, we turned on more ABC passes since my original PRs, so ignore those. But in yosys proper, the largest time sinks are hashing related.

[rmlarsen]

I assume Catherine's question was about hashing in general all over Yosys code, and the associated dict<> and pool<> operations which consume the hashes. Was that what you looked at, Rasmus, or did you mean opt_merge specifically?

But I think Martin fixed the primary source of that (lack of hashing of constants).

FWIW the poor properties of mkhash contributed the most in my testing, which went away when I inserted mkhash_xorshift64 in few key places in the PR here. It may not be the best approach but it's at least something we can improve on.

Ah yes. Hashing in general still dominates the profile:

FWIW, we turned on more ABC passes since my original PRs, so ignore those. But in yosys proper, the largest time sinks are hashing related.

I believe the hot spots in my profile are related to the non-coherent layout of the hash tables in yosys, not time spent in the hash function itself. Replacing the hash table implementation with something like the swisstables from Abseil would likely give a significant speedup. I have not had the time to experiment with this, as it is a rather invasive change. https://abseil.io/about/design/swisstables

[rmlarsen]

@povik here is the flame graph for most of the Yosys cost corresponding to the "Bottom up" list above. It's a mix of OptMergePass/OptExprPass/CleanupPass/OptMuxTreePass.

[povik]

Superseded by Emil's work in #4677 making opt_merge use a new hashing interface