Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wasm/JS BigInt support #10860

Merged
merged 25 commits into from Apr 10, 2020
Merged

Wasm/JS BigInt support #10860

merged 25 commits into from Apr 10, 2020

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 7, 2020

This adds a flag WASM_BIGINT which enables this feature, with
which we let the JS VM use a JS BigInt for a wasm i64. In that case
we don't need to legalize i64s into pairs of i32s.

This is fairly straightforward, but we do need to modify the JS code
of each library method that receives or returns an i64.

Tests verify that we can send and receive i64s from wasm to JS,
and also that a dynCall works. This depends on
WebAssembly/binaryen#2726 for that.

@kripken kripken requested review from sbc100 and jgravelle-google Apr 7, 2020
@@ -3128,6 +3128,9 @@ def legalize_sig(sig):

@staticmethod
def is_legal_sig(sig):
# with BigInt support all sigs are legal since we can use i64s.
if Settings.WASM_BIGINT:
Copy link
Contributor

@jgravelle-google jgravelle-google Apr 9, 2020

Choose a reason for hiding this comment

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

Should legalize_sig be changed to return sig if WASM_BIGINT? That would make this still correct, and handle any other places we'd want the legalized signature

Copy link
Collaborator

@sbc100 sbc100 Apr 9, 2020

Choose a reason for hiding this comment

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

Looks like this is not necessary, since the only callsite is already checking LEGALIZE_JS_FFI before calling.

Replace with assert not Settings.WASM_BIGINT?

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

I think @jgravelle-google is right, since there is another call site to there, so this code path is actually taken with bigints. I missed this in testing since the existing tests happen to not check it carefully enough. Fixed + added a test now, thanks!

Copy link
Collaborator

@sbc100 sbc100 Apr 9, 2020

Choose a reason for hiding this comment

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

But where is the other callsite? I only see one.

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

legalize_sig is called from is_legal_sig and also make_invoke. The latter path was not handled properly and we emitted slightly incorrect invoke code.

Copy link
Collaborator

@sbc100 sbc100 Apr 9, 2020

Choose a reason for hiding this comment

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

is_legal_sig is looks like its only called from emscripten.py:create_fp_accessors

So can't is_legal_sig just do assert not Settings.WASM_BIGINT since calling this when WASM_BIGINT should never happen.

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

Ah, so there are two issues here.

The function @jgravelle-google and I were talking about is legalize_sig. That's not the function this discussion is tacked on to, it's the function @jgravelle-google mentioned we might be missing (and we were).

You are right about is_legal_sig, but I think instead of an assertion it's easy to just do the right behavior as the patch already does? Seems clearer, and ready if we need it later.

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

Oh wait, no, is_legal_sig is called from emscripten.py there, yeah. So we can't assert, we have to do what the patch already does.

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

Oh heh, no, that was wrong, ignore the last comment.

@@ -1390,7 +1390,9 @@ var SyscallsLibrary = {
FS.utime(path, atime, mtime);
return 0;
},
__sys_fallocate: function(fd, mode, off_low, off_high, len_low, len_high) {
__sys_fallocate: function(fd, mode, {{{ defineI64Param('off') }}}, {{{ defineI64Param('len') }}}) {
Copy link
Contributor

@jgravelle-google jgravelle-google Apr 9, 2020

Choose a reason for hiding this comment

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

I can't tell whether this is gross or not.
Either way I like it :)

@@ -2007,6 +2007,9 @@ def check_human_readable_list(items):
# requires JS legalization
shared.Settings.LEGALIZE_JS_FFI = 0

if shared.Settings.WASM_BIGINT:
shared.Settings.LEGALIZE_JS_FFI = 0
Copy link
Collaborator

@sbc100 sbc100 Apr 9, 2020

Choose a reason for hiding this comment

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

Is legalization only dealing with the i64 issue?

Copy link
Member Author

@kripken kripken Apr 9, 2020

Choose a reason for hiding this comment

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

We used to legalize f32s to doubles for JS too, but it turned out that was not necessary. So it is just i64s now, yes.

sbc100
sbc100 approved these changes Apr 9, 2020
Copy link
Collaborator

@sbc100 sbc100 left a comment

Should we call the option JS_BIGINT? Since its a JS feature.

src/support.js Show resolved Hide resolved
tests/return64bit/test.c Outdated Show resolved Hide resolved
tests/return64bit/test.c Outdated Show resolved Hide resolved
tests/return64bit/testbind_bigint.js Outdated Show resolved Hide resolved
tests/test_other.py Outdated Show resolved Hide resolved
@@ -3128,6 +3128,9 @@ def legalize_sig(sig):

@staticmethod
def is_legal_sig(sig):
# with BigInt support all sigs are legal since we can use i64s.
if Settings.WASM_BIGINT:
Copy link
Collaborator

@sbc100 sbc100 Apr 9, 2020

Choose a reason for hiding this comment

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

Looks like this is not necessary, since the only callsite is already checking LEGALIZE_JS_FFI before calling.

Replace with assert not Settings.WASM_BIGINT?

@kripken
Copy link
Member Author

kripken commented Apr 9, 2020

I worry JS_BIGINT isn't clear since it might suggest using bigints in JS for JS reasons. This is really just to use bigints in wasm imports and exports, so it's more about the wasm (and get JS to work with that).

sbc100
sbc100 approved these changes Apr 10, 2020
@kripken kripken merged commit 42cbd6c into master Apr 10, 2020
34 checks passed
@kripken kripken deleted the bigint branch Apr 10, 2020
@littledan littledan mentioned this pull request Jun 23, 2020
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.

None yet

3 participants