Neater hashing interface

[widlarizer]

Please read docs/source/yosys_internals/hashing.rst for what the interface is now.

We want to be able to plug-and-play hash functions to improve hashlib structure collision rates and hashing speed. Currently, in some ways, hashes are incorrectly handled: for deep structure hashing, each substructure constructs a hash from scratch, and these hashes are then combined with addition, XOR, sometimes xorshifted to make this less iffy, but overall, this is risky, as it may degrade various hash functions to varying degrees. It seems to me that the correct combination of hashes is to have a hash state that is mutated with each datum hashed in sequence. That's what this PR does.

• check performance impact since this PR isn't NFC as it changes how structures are hashed
• clean up things left over from experiments with inheriting from Hashable
• fix pyosys
• finish the plugin compatibility part of the doc

[widlarizer]

Cool, I got hit in the face with a downright gcc bug

[widlarizer]

With ibex and jpeg synthesized with ORFS, I'm seeing a 1% performance regression with this PR. This is probably because we're actually using the seed function more directly, with less xorshifting involved. I wonder if a quick swap of the hashing function would change the result. However, I'm also seeing a 1% memory usage improvement with jpeg, which is pretty interesting

[widlarizer]

Passes like extract_fa and opt_dff take 5-10% longer with this change. This is definitely a problem

[povik]

Leaving a note so I don't forget it: We should provide a way for plugins to be compatible with both pre and post this change. I am thinking a define to advertise the new API.

[jix]

Despite the many comments I left, I really like the new hashing interface and think it's a big improvement over the previous one.

I found a bunch of nits, made a few suggestions on how to potentially improve some of the hash_eat implementations, am slightly confused by the current choice of state update in the used hasher implementation and found one instance of unconditionally recomputing an already cached hash.

edit: Oh, and to clarify, I'm fine with deferring my suggestions that would need some more benchmarking to a follow up PR and merge this once everything that's a clear issue right now or a clear benefit to change is addressed.

[jix]

I was expecting to only see a djb hash update in here, but it is followed up by a xorshift. What's the reason for doing both? Without benchmarking I would be guessing that performing a djb hash update and a xorshift update is slower than either alone without necessarily having better overall runtime behavior than the better behaved one of both.

[widlarizer]

In an intermediate state, I found it necessary for lowering collisions and it doesn't cost so much as to cause a measurable regression against main on the ASIC designs I used to check. Without it, DJB2 doesn't have an avalanche property. Also see this observation basically about patterns like hash = mkhash(xorshift(mkhash(a, b)), c)

[jix]

Sadly moving this turns the diff into a mess. Are there any functional changes to this besides updating the hashing implementation?

[widlarizer]

The problem is that rtlil.h both creates per-type partial specializations for hash_top_ops defined in hashlib.h as well as uses data structures defined in hashlib.h. No functional changes other than hash_top_ops and hash_eat.

[jix]

assuming my suggestion of hashing strings in chunks is implemented, the vector of State enums could also be hashed as if it were a string. If this is using a string representation the same is true. I'm assuming this would cause issues when we have two equal consts, one represented as states one as string. I still have to take a closer look at how the new string representation for constants is implemented to say more on this.

[widlarizer]

We need a string represented as string vs as bit vector to have equal hashes, so we have to go bit by bit. To hash a contiguous chunk, for string represented Const, we'd need to first construct a new bit vector to run it on that. So it doesn't sound like it pays off

[jix]

Yeah, that's what I meant with "I'm assuming this would cause issues when we have two equal consts, one represented as states one as string". I have some ideas on how to compute a hash that processes more than a bit at a time while still agreeing between the two representations. That would be quite a bit more complex though, so probably only worth it if hashing consts is a significant overall cost. Do you happen to have any data on that specifically? In any case, this would be something we can do in a later PR.

[widlarizer]

Ah sorry, I misread that. This is an interesting challenge. I haven't noticed hashing Consts as a hotspot so far

[jix]

what I wrote for the c string hash_eat below above also applies here, but I initially missed that std::string is handled here

[povik]

I don't understand, if I compile my plugin against v0.47 or earlier I won't have Hasher

[widlarizer]

I replaced this with an interface versioning macro solution

[povik]

Leaving a note that I read this file

[povik]

Is it to get lower hash collisions or to get better locality?

[widlarizer]

Hash collisions

[povik]

Does this come from observations, or something Claire mentioned was intention? I know some of the primitives were used in a way to get better locality

[widlarizer]

This comes from my observations when counting hash collisions in hashlib per std::source_location and clear correlation with extra runtime overhead in some opt and extract_fa passes where the collisions were happening

[jix]

FWIW, cpython also gives reducing collisions as a reason for using small integer values directly as hash value: https://github.com/python/cpython/blob/7538e7f5696408fa0aa02fce8a413a7dfac76a04/Objects/dictobject.c#L289

[widlarizer]

Thanks for the resource. I'm perfectly fine with hashing an int trivially, but retaining patterns in the output when hashing a combination of them sketches me out. cpython:

>>> bin(hash((1, 2)))
'-0b11000101000100010101000101001111011111101000000110000010111101'
>>> bin(hash((1, 3)))
'-0b1001111111110101001100100011001110110010011111111100101100100'

But for SigBit we get this:

>>> print(djb2_add(1, 2))
35
>>> print(djb2_add(1, 3))
36
>>> print(djb2_add(2, 3))
69

[widlarizer]

Thanks @jix and @povik for your reviews of the large amount of code change. I don't think I've left anything unaddressed. Since we're nearing the end of the release cycle, we'll have to target the next one

[povik]

This change hid hash_ptr_ops from the Yosys namespace causing an avoidable API breakage. See povik/yosys-slang@a2e91d4. We can walk that back before the release.

[povik]

See povik/yosys-slang@a2e91d4.

This patch isn’t enough to be compatible post the API change. I am not sure how to declare the dicts in a way which works on both sides of the API change without resorting to preprocessor logic.

[widlarizer]

I could globally add using hashlib but that would defeat the whole point of a namespace I guess. Just beware that prior to this PR, YS_HASHING_VERSION was undefined, not set to 0

[povik]

Just beware that prior to this PR, YS_HASHING_VERSION was undefined, not set to 0

I assume you are mentioning this because I've written #if YS_HASHING_VERSION <= 0. That should be fine, I've read the C preprocessor is specified to interpret undefined macros as zeroes in expressions.

AIUI to make the below, which has worked before the PR, to also work post the hashing change

	Yosys::pool<const ast::Statement *, Yosys::hash_ptr_ops> loops;

we need to (1) put using hashlib::hash_ptr_ops; back into the Yosys namespace and (2) adapt the pool and other templates to accept a hash_ops-like type, not the hash_top_ops type

[widlarizer]

I guess I could rename hash_ops to hash_eat_ops and hash_top_ops to hash_ops. It wouldn't hurt boilerplate very much within the project. I would then provide using within the Yosys namespace for neither. Plugin devs who need it could - conditionally on YS_HASHING_VERSION - raise hash_ops out of Yosys::hashlib into Yosys like this. This way we retain the distinction between top level object hashing and component hashing and still retain encapsulation

[povik]

I’m not clear on how that helps. It would still be an error to put hash_ptr_ops for a template argument of pool or dict, right?

[widlarizer]

Right. I don't know of a way to fix that then

[widlarizer]

adapt the pool and other templates to accept a hash_ops-like type, not the hash_top_ops type

This would be in conflict with the purpose of this whole PR, if I'm understanding your suggestion correctly

[povik]

Can't we make the template take an OPS argument with a hash_into method?

[widlarizer]

Since the declaration of dict is currently template<typename K, typename T, typename OPS = hash_top_ops<K>> class dict; I can't deny a theoretical possibility of resolving this but I don't personally see any way of overriding an internally used top-like interface by passing in a template parameter of a different interface