fix hashWangYi1 when nimvm result to 0

[bung87]

reason same as hiXorLo

run nim js -r a.nim

code example:

import hashes
const h2 = hashWangYi1('a'.uint64)
echo "const:",h2
let h4 = hashWangYi1('a'.uint64)
echo "runtime:",hashWangYi1('a'.uint64)

[ringabout]

What's the issue?
Or the testcase for the issue?

[cooldome]

For the code snippet above I am getting the following output:

const:661948602591176288
runtime:661948602591176288

I don't see issues here. Could you please explain?
Possibly it is platform specific, what is your compilation target?

[bung87]

For the code snippet above I am getting the following output:

const:661948602591176288
runtime:661948602591176288

I don't see issues here. Could you please explain.

do you run it target js, the const will go to when nimvm branch ,it need result= , after adding it you still get a number differs from js runtime result. and you check CI tests all fails.

I create a pr for issue this, please check #15933

[cooldome]

I see it is the javascript backend is affected.
Without this fix I get:

const:0
runtime:4423056995083872

with fix I am getting:

const:889415264
runtime:4423056995083872

It is still different but getting better. It needs to match though.

[c-blake]

The lower 32-bits do match (probably everyone knows, but bears mentioning - print the numbers in hex and you will see the lower 8 nibbles match).

I 100% agree, though, that we should get the whole number matching. That JS hash is actually pretty slow and has its current definition specifically to match the C/C++ backends as much as possible.

Also, while this result = workaround is fine, it probably speaks to a deeper/more general compiler bug, possibly related to having result = in the asm but not in the when nimvm and/or default else: branches. (Just guessing...). Someone may want to create a more isolated test case for that and file an issue so it does not get forgotten.

[c-blake]

Oh, wait..I just read my comment in the asm. Lol. We are only supposed to expect matching the lowest 32-bits. (tables and sets use and to range reduce to table size. So, this will match for up to 4 GiEntry tables.) But I would still put result = in all 3 when branches to be consistent, and still try to isolate the broader problem.

[bung87]

when nimvm has problem when it only has one branch, there's a issue for that, the problem here is caculate at compile time target to js , js has correct version , while vm not, if only low 32bit matters , maybe need a custom == operator to handle this.

[c-blake]

Hmm. The when nimvm here all have 2 branches. Maybe you need the result = here because proc hiXorLo also does result =? There still seems like a deeper issue (though there may already be a github issue for it).

If matching is important then we could also mask the high bits to zero (just for the JS target and just in the nimvm mode). The idea behind this limited 32-bit range on JS was that anyone doing giant 4GiEntry*8..32B = 32..128 GiB single tables was also probably enough of a power user to just define their own hash. That's kind of a footgun until Nim uses BigInt as a backend for JS integers, not the JS "number".

[ringabout]

Related issue:
#15624

And it's better to move tests from #15933 to here.

[bung87]

haha , dont mind when nimvm issue , this PR just proven hashWangYi1 not get result default value (0), that test case will cover these two problem, here mainly is focus on result to zero . c-blake make your fix, this PR doesn't matters.

[cooldome]

I believe result = is needed because result symbol in js branch is used explicitly. Please also add result = on line 134 as well.
Please investigate how to make hashes in vm and jsgen aligned.

[bung87]

Close in favour of #15937