Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

kripken · 2023-06-09T20:36:33Z

When a JS library calls into wasm and the function has an i64 argument, we must
legalize into two 32-bit integers if we lack BigInt support.

Note that we may be able to do more efficient things than splitI64 (assuming
the input uses only 53 bits, and is signed, can help simplify things a lot), but we
leave that for optimization later.

This removes the old/unused sendI64Argument.

sbc100

Is there a reason you need/want this?

sbc100 · 2023-06-09T22:11:14Z

src/parseTools.js

  }
-  return low + ', ' + high;
+  return `(${name} | 0), (${name} / (2**32))`;


This seems like it should live in src/library_int53.js?

It's such a tiny code fragment that I think it can stay in the helper function here? Pulling it out would then mean writing deps for it etc.

I do agree that if we add checking then that would be pulled into a library function, as that is no longer trivial.

Maybe add a comment saying that this function logically is in the same family as the functions in src/library_int53.js

The code in library_int53.js uses expanded out numbers rather than 2**32.

You could do that here by putting the 2**32 part inside a ${} ?

Wait, why are we expanding them there? It's larger to expand. I think we should 2**32ify them?

Well we should at least be consistent. I don't really care which form we use. If you want to use the expanded form in the PR and followup with change to the runtime multiple everywhere that is also fine be me.

kripken · 2023-06-09T22:26:12Z

Sorry @sbc100 , we discussed this a few days ago but I should have written the context. @jameshu15869 has a few PRs up for WasmFS in which the WasmFS JS API needs to call into wasm, say to do truncate(). One of the params there is a 64-bit integer (because a file can be larger than 32 bits), so this PR would unblock that work.

jameshu15869 · 2023-06-12T13:37:59Z

I tried using this PR with the truncate() issue and positive numbers were being passed correctly. However, I tried the library_i64_params test with -42, and the C function still was not interpreting the -42 correctly. The log is shown below.

+js:got:       -42
+This param:  -42 -9.778887033462524e-9
+called_from_js with: 4294967254

This param: was a log I added to see the actual pair of ints, and I noticed that the part that was divided by 2**32 was close to zero but not exactly zero. I added Math.floor(${name} / 2**32) and the function seemed to work for negative numbers after that. I'm not sure that this is the right fix, since the second int in the pair is now -1 instead of 0, which I expected. Do you guys have any suggestions for a better fix?

kripken · 2023-06-12T20:16:45Z

After discussion with @tlively and @jameshu15869 , the first thing we need is the signed version actually. I uploaded that now, and also moved the function to the JS library as you requested @sbc100

@jameshu15869 I added a test for -42 here specifically, which now seems to pass.

With that said, I had thought the math here was trickier than it turned out when I did it now... so I worry I'm forgetting something. But perhaps the tricky part is to handle things outside of the i53 range, which we don't care about...

sbc100 · 2023-06-12T20:27:04Z

src/library_int53.js

+global.i53ConversionDeps = [
+  '$convertI32PairToI53Checked',
+  '$sendI53BitsLow',
+  '$sendI53BitsHigh',


I'm not sure about this since the latter two will very rarely be used.. unlike the first one which is basically used everywhere.

sbc100 · 2023-06-12T20:29:42Z

src/library_int53.js

+    // TODO: assert on num being in the valid i53 range
+    num = Math.floor(num / 4294967296);
+    return num | 0;
+  },


Why not make make a single I53 split function that returns the high/low pair?

Now that I think of that I remember there is an existing splitI64 in parseTools.js.. maybe we don't want to keep both?

yeah for now maybe just use splitI64 and we can think of simplifying it later?

I think that method is large and complex because (1) it handles non-i53 values and (2) it isn't clear on whether the input is signed or unsigned, and I think it tries to handle both. This PR's method assumes the input is a signed i53 which simplifies things a lot.

For the purposes of the change in question isn't splitI64 OK though? If you want to add an alternative to splitI64 maybe just add splitI53 alongside it with a comment as to why its different? Perhaps we don't need splitI64 anymore at all? I'd like to avoid duplicating its functionality though.. maybe this is good change to clean things up.

How about this:

Land the current wasmfs changes using the existing splitI64

Look into optimizing/removing/refactoring splitI64 as a followup

Ok, if you prefer that I don't feel strongly. I changed it to that.

When we look at code size for WasmFS perhaps we can get back to this.

juj · 2023-06-12T21:21:44Z

src/library_int53.js

+  $sendI53BitsHigh: function(num) {
+    // TODO: assert on num being in the valid i53 range
+    num = Math.floor(num / 4294967296);
+    return num | 0;


Do we use the send and receive naming still widely? I always thought those terms confusing, since from static reading of an individual function, it is not clear where the data is supposed to be flowing, and what side should be considered the sender, and which one should be regarded the receiver.

I know there is a comment there to explain, but it seems like the name choice makes the comment mandatory?

It would be nice to avoid send and receive terms if possible. Maybe these could be named getLowI32Part(num) and getHighI32Part(num) instead?

The impl could read Math.floor(num / 4294967296) | 0; without loss on readability?

For the TODO, maybe expanding the comment with

// You can use assert(Number.isSafeInteger(num)); to verify that a given number // can be converted without precision loss.

could be used, so the caller can emit or not emit such an assert, depending on their use case whether they accept lossy conversion or not?

Thanks for the feedback! Meanwhile though I just removed all this code for now. But we may get back to it as an optimization later.

I do agree the names are ambiguous. Perhaps forCallToWasm or such would make it more clear..?

Whats wrong with just splitI53?

sbc100

LGTM with PR title and description updated.

fix

37d2a42

kripken requested a review from sbc100 June 9, 2023 20:36

sbc100 reviewed Jun 9, 2023

View reviewed changes

kripken added 3 commits June 12, 2023 13:02

Merge remote-tracking branch 'origin/main' into js2wasm.i64_param

5f530ca

signed

561f155

test -42 as well

9f67e91

kripken changed the title ~~Add sendU53ToI64Param() helper for JS libraries to call into wasm~~ Add sendI53ToI64Param() helper for JS libraries to call into wasm Jun 12, 2023

TODOs

0da5609

sbc100 reviewed Jun 12, 2023

View reviewed changes

juj reviewed Jun 12, 2023

View reviewed changes

kripken added 2 commits June 12, 2023 14:24

test

c979f28

undo and use splitI64

abac05f

sbc100 approved these changes Jun 12, 2023

View reviewed changes

kripken changed the title ~~Add sendI53ToI64Param() helper for JS libraries to call into wasm~~ Add testing for JS libraries calling into wasm [NFC] Jun 12, 2023

fix.bigint

d45e7ce

kripken changed the title ~~Add testing for JS libraries calling into wasm [NFC]~~ Fix splitI64 so it can be used to send values from JS libraries into wasm Jun 12, 2023

sbc100 approved these changes Jun 12, 2023

View reviewed changes

kripken merged commit 95ab28e into main Jun 12, 2023

kripken deleted the js2wasm.i64_param branch June 12, 2023 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

kripken commented Jun 9, 2023 •

edited

Loading

sbc100 left a comment

sbc100 Jun 9, 2023

kripken Jun 9, 2023

sbc100 Jun 9, 2023

sbc100 Jun 9, 2023

kripken Jun 10, 2023

sbc100 Jun 10, 2023

kripken commented Jun 9, 2023

jameshu15869 commented Jun 12, 2023

kripken commented Jun 12, 2023

sbc100 Jun 12, 2023

sbc100 Jun 12, 2023

sbc100 Jun 12, 2023

kripken Jun 12, 2023

sbc100 Jun 12, 2023

kripken Jun 12, 2023

juj Jun 12, 2023

kripken Jun 12, 2023

sbc100 Jun 12, 2023

sbc100 left a comment

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

Conversation

kripken commented Jun 9, 2023 • edited Loading

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jun 9, 2023

jameshu15869 commented Jun 12, 2023

kripken commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

kripken commented Jun 9, 2023 •

edited

Loading