Skip to content

Enable Wasm BigInt#2643

Merged
hoodmane merged 17 commits intopyodide:mainfrom
hoodmane:wasm-bigint
Jun 2, 2022
Merged

Enable Wasm BigInt#2643
hoodmane merged 17 commits intopyodide:mainfrom
hoodmane:wasm-bigint

Conversation

@hoodmane
Copy link
Copy Markdown
Member

@hoodmane hoodmane commented May 30, 2022

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

@hoodmane
Copy link
Copy Markdown
Member Author

Well scipy is not happy about this.

hoodmane added a commit to hoodmane/pyodide that referenced this pull request May 30, 2022
It causes trouble when these flags get out of sync, cf
WASM_BIGINT branch pyodide#2643
@hoodmane
Copy link
Copy Markdown
Member Author

Okay, looks like it works now =)

@hoodmane
Copy link
Copy Markdown
Member Author

This change reduces the size of pyodide.asm.js, pyodide.asm.wasm, and clapack each by about 100kb. No measured effect on the benchmarks.

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an explanatory comment for this ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 $v = a_{N-1}2^{N-1}+\sum a_{i}2^{i}$ and we want $w = -a_{N-1}2^{N-1}+\sum a_{i}2^{i}$ we have to subtract $2\cdot a_{N-1}2^{N-1}$. Here $N = 32$. Confusingly, in JavaScript if you take a positive number between $+2^{31}$ and $+2^{32}$ and compute x & 0x80000000, the result is $-2^{31}$ so we need to add 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.

Copy link
Copy Markdown
Member Author

@hoodmane hoodmane Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@hoodmane hoodmane Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it sounds more complicated than I would have expected.

Should we add some tests for IBIGINT_FROM_PAIR in JS?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it turns into dead code once we enable WASM_BIGINT: it's in the #else block of #if WASM_BIGINT.

@hoodmane
Copy link
Copy Markdown
Member Author

I split off two PRs from this one: #2650 and #2648.

hoodmane added a commit that referenced this pull request May 31, 2022
It causes trouble when these flags get out of sync, cf
WASM_BIGINT branch #2643
@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Jun 1, 2022

According to caniuse this is dropping support for ~0.7% of users.

@rth
Copy link
Copy Markdown
Member

rth commented Jun 2, 2022

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?

@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Jun 2, 2022

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.

@rth
Copy link
Copy Markdown
Member

rth commented Jun 2, 2022

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.

@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Jun 2, 2022

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.

@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Jun 2, 2022

Thanks for the review @rth!

@hoodmane hoodmane merged commit 95b1194 into pyodide:main Jun 2, 2022
@hoodmane hoodmane deleted the wasm-bigint branch June 2, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants