Conversation
|
Well scipy is not happy about this. |
Add PYODIDE_SYMBOLS and PYODIDE_SOURCEMAP environment variables to adjust debug symbol settings. Add PYODIDE_ASSERTIONS to set -DDEBUG_F.
It causes trouble when these flags get out of sync, cf WASM_BIGINT branch pyodide#2643
|
Okay, looks like it works now =) |
|
This change reduces the size of |
rth
left a comment
There was a problem hiding this comment.
Great that you managed to make it work! Let's maybe wait for feedback to your emscripten PR, other than that LGTM (assuming the debug-related lines are reverted).
| (BigInt(lower) | (BigInt(upper) << BigInt(32))) | ||
|
|
||
| #define IBIGINT_FROM_PAIR(lower, upper) \ | ||
| (BigInt(lower) | (BigInt(upper - 2 * (upper & 0x80000000)) << BigInt(32))) |
There was a problem hiding this comment.
Could you add an explanatory comment for this ?
There was a problem hiding this comment.
Yeah I will add a comment. To quote wikipedia:
https://en.wikipedia.org/wiki/Two%27s_complement#Converting_from_two's_complement_representation
The value w of an N-bit twos-complement integer
${\displaystyle a_{N-1}a_{N-2}\dots a_{0}} a_{N-1} a_{N-2} \cdots a_0$ is given by the following formula:
$w=-a_{N-1}2^{N-1}+\sum a_{i}2^{i}$
So if we have x & 0x80000000, the result is 2*(x & 0x80000000).
I don't really understand how bitwise operators work in JavaScript, I always have to use a lot of trial and error. Everything is actually floats so to do bitwise ops it converts from floats to... I'm not sure what size integers and then does the bit ops and then converts everything back to floats. Since there isn't really a leading bit, I don't understand what determines when the result of a bitwise operation is positive or negative. Maybe I should read the ECMAScript spec. The rules for BigInts are equally confusing and also different.
There was a problem hiding this comment.
In particular, my major point of confusion here is that in ordinary signed arithmetic, x >= 0 implies (x && y) >= 0. This is because the property of being <0 is the same as "the sign bit is set" and bitwise && doesn't set any new bits. But this is NOT true of JavaScript bitwise and.
There was a problem hiding this comment.
Okay further experimentation gives the answer: when you do any bitwise operation, the floats are cast to i32 and then the bitwise operation occurs and then they are cast back to float. So (2**32) | 0 is 0, (2**32 - 1) | 0 is -1. JavaScript is weird.
There was a problem hiding this comment.
yes, it sounds more complicated than I would have expected.
Should we add some tests for IBIGINT_FROM_PAIR in JS?
There was a problem hiding this comment.
Well it turns into dead code once we enable WASM_BIGINT: it's in the #else block of #if WASM_BIGINT.
It causes trouble when these flags get out of sync, cf WASM_BIGINT branch #2643
|
According to caniuse this is dropping support for ~0.7% of users. |
Is the limit on the JS bigint support in general, or are there specific WASM bigint features that need to be supported? |
|
Just js features, 64 bit integer support is part of the wasm mvp. But they are awkward if JavaScript doesn't support them because so many things touch the wasm/js interface. |
|
https://caniuse.com/bigint in terms of supported browser versions looks very reasonable. We don't support Safari 13 anway. Looks like upstream is happy with your patch :) Thanks for doing it. LGTM as well once the patch is updated to match your upstream PR. |
|
I already backported the most up-to-date version of the patch. It's not a 100% clean backport because some of the compiler guards changed between 2.0.27 and tip of tree. |
|
Thanks for the review @rth! |
This enables WASM_BIGINT while maintaining (hypothetical) Safari 14 support
by shimming BigInt64Array and BigUint64Array if they are missing. I think the
last time we tried to enable WASM_BIGINT was before #2019 so our chances
are significantly better this time.
This will fix dynamic linking bugs, reduce code size, and improve speed.
See also my upstream PR:
emscripten-core/emscripten#17103