Enable BigInt for binaryen.js #7167
Conversation
We previously had a build option for enabling BigInt support when compiling with Emscripten, but the test suite did not actually pass with it enabled. The problem was that the binaryen.js glue code assumed that C API functions that took i64 parameters would be transformed to take a pair of i32 parameters instead, but this assumption was incorrect when BigInt support was enabled. Emscripten has now enabled BigInt by default, so update binaryen.js to use BigInt as well. Fix the JS API glue code to pass i64s as BigInts rather than pairs of numbers and fix the tests accordingly. Also fix some other small problems with the tests that only show up in debug builds. Resolves #7163.
| return preserveStack(() => { | ||
| const tempLiteral = stackAlloc(sizeOfLiteral); | ||
| Module['_BinaryenLiteralInt64'](tempLiteral, x, y); | ||
| Module['_BinaryenLiteralInt64'](tempLiteral, BigInt(x)); |
There was a problem hiding this comment.
What do you think about making this backwards compatible? We could support (i32, undefined), (i32, i32), and (BigInt, undefined) with this code:
// Convert to a BigInt. We support (x, undefined) where x is either a
// BigInt or not, and also (x, y) where neither is a BigInt and y is the
// higher bits.
if (typeof x !== 'bigint') {
x = BigInt(x || 0) | (BigInt(y || 0) << 32n);
}There was a problem hiding this comment.
A small code size cost seems reasonable to me here, personally. And it is nice to write binaryen.i64.const(42) and not need to remember 42n each time.
There was a problem hiding this comment.
binaryen.i64.const(42) already works because of the explicit BigInt(x) conversion here. Except for compatibility, I don't think there's much benefit to supporting the two-argument version. How much do we value compatibility here? Would we have a deprecation plan, or would we support the two-argument version indefinitely?
There was a problem hiding this comment.
I think compatibility makes sense: there are users that are creating i64s with two arguments, and I don't see a strong reason to break them, given the burden on supporting the old format is minimal.
|
"em++: error: WASM_BIGINT=1 is not compatible with WASM=0 (wasm2js)" seems like a problem... Do we need to drop the JS build? Do we need to use WASM_BIGINT=0 forever? Is there a third option? |
|
About the JS build, that makes me worried actually... I don't know offhand how important JS builds are for binaryen specifically, but in general, I would guess there are people that need JS builds. If they need both JS and wasm builds then if wasm2js doesn't support bigint, that means they'd need to either
This makes me think we should just support bigint with wasm2js. We don't need full bigint support in codegen - only to make imports and exports from the wasm be able to convert from bigints on the outside into cc @dschuff - what do you think? If this makes sense, maybe we should do it before releasing the first Emscripten version with bigint by default. |
|
Yeah, I think adding BigInt support for wasm2js by converting BigInt <-> Number pairs on the boundaries makes sense, since as you say it would let people use the same interface for JS and wasm. |
|
I think if we make wasm2s support bigint then the "bigint by default" update would become frictionless/unnoticeable for most users. People doing manual bindings like binaryen.js will need updates as in this PR, but I'm hoping that's rare. |
We previously had a build option for enabling BigInt support when
compiling with Emscripten, but the test suite did not actually pass with
it enabled. The problem was that the binaryen.js glue code assumed that
C API functions that took i64 parameters would be transformed to take a
pair of i32 parameters instead, but this assumption was incorrect when
BigInt support was enabled.
Emscripten has now enabled BigInt by default, so update binaryen.js to
use BigInt as well. Fix the JS API glue code to pass i64s as BigInts
rather than pairs of numbers and fix the tests accordingly. Also fix
some other small problems with the tests that only show up in debug
builds.
Resolves #7163.