1279876 - Wasm baseline: Correct truncate-to-int32 semantics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Lars T Hansen [:lth]]

Spun off from bug 1232205.  Benjamin writes:

>@@ +2727,5 @@
>> +      public:
>> +        OutOfLineTruncateFOrF64ToI32(AnyReg src, RegI32 dest) : src(src), dest(dest) {}
>> +
>> +        virtual void generate(MacroAssembler& masm) {
>> +            bool isAsmJS = true; // Wasm passes for AsmJS in this context
>
>I think this is not correct, see below...
>
>@@ +2742,5 @@
>> +    bool truncateF32ToI32(RegF32 src, RegI32 dest) {
>> +        OutOfLineCode* ool = addOutOfLineCode(new (alloc_) OutOfLineTruncateFOrF64ToI32(AnyReg(src), dest));
>> +        if (!ool)
>> +            return false;
>> +        masm.branchTruncateFloat32(src.reg, dest.reg, ool->entry());
>
>Actually, AsmJS and Wasm don't have the same truncation semantics: asm.js does a >truncation modulo the int32 range (try assertEq(2**32 + 1 | 0, 1) in a shell), >but in Wasm, it's an invalid conversion. (See EmitTruncate in WasmIonCompile.cpp)
>
>So you just need to use wasmTruncateF32ToInt32 or branchTruncateFloat32 whether >we're in wasm or asm.js, and have the correct OOL generated too.
>
>@@ +2752,5 @@
>> +    bool truncateF64ToI32(RegF64 src, RegI32 dest) {
>> +        OutOfLineCode* ool = addOutOfLineCode(new (alloc_) OutOfLineTruncateFOrF64ToI32(AnyReg(src), dest));
>> +        if (!ool)
>> +            return false;
>> +        masm.branchTruncateDouble(src.reg, dest.reg, ool->entry());
>
>ditto

We have no test cases that expose this bug, and need to have them.

I opted to fork this off as a separate bug since it could look like some refactoring work is needed in the back-ends to share code with the CodeGenerator.

Comment 1

[Lars T Hansen [:lth]]

Benjamin believes we have test cases for this, but all my test running has not tripped across any problems, even on x86 (as opposed to x64).

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #1)
> Benjamin believes we have test cases for this, but all my test running has
> not tripped across any problems, even on x86 (as opposed to x64).

These tests are in the spec test files, disabled because of bug 1248555.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: tests.patch

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.refactoring.patch

This refactors the wasmTruncate and outOfLineWasmTruncate methods:
- to not take a type parameter anymore (each type combination gets its own method)
- and puts them in the MacroAssembler high-level sections.

This doesn't remap the functions on ARM, as they are not needed yet.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.fix-baseline.patch

This fixes the baseline compiler to match the correct semantics. There are quite a few #ifdef, but they could be removed as we support other platforms.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.test.patch

These are the tests that would have failed before and don't fail with the previous patch. Also renames basic-conversion to conversion, because the basic- prefix doesn't make sense.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8775164 [details] [diff] [review]
1.refactoring.patch

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

This looks OK to me, so r+, but I don't really have the headspace until next week for a deep review, so if you're nervous about it maybe ask Hannes.

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8775166 [details] [diff] [review]
2.fix-baseline.patch

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

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +2523,5 @@
>        public:
>          OutOfLineTruncateF32OrF64ToI32(AnyReg src, RegI32 dest)
>            : src(src),
> +            dest(dest),
> +            isAsmJS(true),

It would IMO be better to have a single constructor with four arguments - explicit is better than implicit.  It could assert !asmJS || !isUnsigned, for example, to weed out abuse.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4526ff61ff1f
Refactor wasmTruncateToInt32 methods into the masm; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a009910666d3
Implement correct semantics of TruncateToInt32 in the wasm baseline compiler; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2db5a60751
Test cases; r=lth

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4526ff61ff1f
https://hg.mozilla.org/mozilla-central/rev/a009910666d3
https://hg.mozilla.org/mozilla-central/rev/ee2db5a60751