Closed Bug 1431416 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch handleundefined.patch (obsolete) — Splinter Review
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.
Attachment #8943621 - Flags: review?(jdemooij)
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.
Attachment #8943621 - Flags: review?(jdemooij)
Attached patch 1.dead.patchSplinter Review
Attachment #8943621 - Attachment is obsolete: true
Attachment #8943693 - Flags: review?(luke)
Attached patch 2.truncate.patchSplinter Review
From what I understand, using truncation is fine here, but well, who knows. At least wasm and asm.js tests agree.
Attachment #8943694 - Flags: review?(luke)
(Repurposing this bug)
Summary: Jit: fast path undefined conversion in MacroAssembler::convertValueToInt32 → wasm: use truncation in the wasm->jit return value path
Blocks: wasm
Attachment #8943693 - Flags: review?(luke) → review+
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.
Attachment #8943694 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/728e277830cc
https://hg.mozilla.org/mozilla-central/rev/b6d4b546fa1f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: