1431416 - wasm: use truncation in the wasm->jit return value path

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: handleundefined.patch

MacroAssembler::convertValueToInt32 ignores undefined for the regular conversion path, although it shouldn't, I think.

This is an issue especially in the jit->wasm path, where we use convertValueToInt32 in the entry to unbox int32 arguments; and every time we'll use an arguments rectifier (arguments underflow), then the very slow path (calling back to C++) is taken, which is unfortunate.

Also removes a few dead methods related to int32 conversion in the MacroAssembler.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8943621 [details] [diff] [review]
handleundefined.patch

Probably evilpie is correct and convertToInt32 shouldn't do this.

I don't clearly understand what's the difference between the truncating and non truncating behavior is; moreover, the try build I sent yesterday with this change didn't break anything. There's a comment atop of MToInt32 and MTruncateToInt32, but they don't really explain the difference neither. Giving up on this patch; I'll do another to remove the unused methods and use truncation in wasm.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.dead.patch

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.truncate.patch

From what I understand, using truncation is fine here, but well, who knows. At least wasm and asm.js tests agree.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(Repurposing this bug)

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8943694 [details] [diff] [review]
2.truncate.patch

Review of attachment 8943694 [details] [diff] [review]:
-----------------------------------------------------------------

It sounds like truncation matches the ToInt32() spec conversion which is what we have in asm.js (via |0) and wasm (via https://webassembly.github.io/spec/js-api/index.html#towebassemblyvalue), so this lgtm.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/728e277830cc
Remove dead methods in the MacroAssembler; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d4b546fa1f
Use truncation in wasm->jit return path; r=luke

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/728e277830cc
https://hg.mozilla.org/mozilla-central/rev/b6d4b546fa1f