Closed Bug 1336139 Opened 3 years ago Closed 3 years ago

wasm: differential testing issue with f64.convert_u/i64

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

const wasmEvalText = (txt, maybeImports) => new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(txt)), maybeImports);

setJitCompilerOption('wasm.test-mode', 1);

function printValue(val) {
    if (typeof val === 'number') {
        print(val);
    } else if (typeof val === 'object') {
        if (typeof val.nan_low !== 'undefined') {
            print('nan_low:', val.nan_low);
            print('nan_high:', val.nan_high);
        } else if (typeof val.low !== 'undefined') {
            print('i64_low:', val.low);
            if (typeof val.high !== 'undefined')
                print('i64_high:', val.high);
        } else {
            print('// unknown object type');
            print(JSON.stringify(val));
        }
    } else {
        print('// unknown type');
        print(val);
    }
}

let exports;
try {
    exports = wasmEvalText(`
    (module
        (func (export "func_0") (result f64)
            ;; Top level expr.
            i64.const 13800889852755076097
            ;; Top level expr.
            f32.const -13.37
            f32.const -13.37
            i32.const 3772612476
            select
            f32.const 0.000000004
            f32.floor
            i32.const 1043365186
            i32.const 1022046386
            i32.const 3956871783
            select
            select
            ;; Top level expr.
            i64.const 11706110227077245217
            i64.clz
            i64.popcnt
            ;; Top level expr.
            i64.const 3473971560141998371
            i64.popcnt
            f32.const 0.000000004
            f32.const 3.402823669209385e+38
            f32.ge
            select
            ;; Top level expr.
            i32.const 1834887630
            ;; Emptying the result stack.
            f64.convert_u/i32
            drop
            i32.wrap/i64
            drop
            i32.trunc_s/f32
            drop
            f64.convert_u/i64
        )
    )
    `).exports;
} catch(e) {
    print(e);
    throw new Error('INVALID_TESTCASE');
}

try {
    
// Printing globals.
// Calling into functions.
print("function 0:");
printValue(exports.func_0())

} catch(e) {
    print(e);
}

/*
x86:
function 0:
13800889852755077000

x64:
function 0:
13800889852755075000
*/
Reduced to:

i64.const 13800889852755076097
f64.convert_u/i64
Summary: wasm: differential testing issue → wasm: differential testing issue with f64.convert_u/i64
[Tracking Requested - why for this release]: incorrect behavior in wasm
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attached patch fix.patch (obsolete) — Splinter Review
In the above example, x64 would return 13800889852755075000, when the expected result is 13800889852755077000.

The issue is in the int64->f64 conversion. Since the input is signed, we end up here: http://searchfox.org/mozilla-central/source/js/src/jit/x64/MacroAssembler-x64.cpp#117-123

Rounding errors can show up here. Intuitively, if we preserve the LSB bit after the shr, re-adding the integer input twice would lead to something closer to the original value; thus converting to float64 and doing the addsd leads to something close to the original value. See also https://codereview.chromium.org/1435203003/ where a similar patch is done.

(fwiw, there were 0.5% instances of this error in the last round of wasm fuzzing I've done)
Attachment #8834068 - Flags: review?(sunfish)
Tracking wasm correctness issue for 52/53
Comment on attachment 8834068 [details] [diff] [review]
fix.patch

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

The f32.convert_u/i64 variant doesn't need this change, because the least significant digit of a very large unsigned i64 value is lost in rounding to an f32.

FYI, the spec interpreter has a similar bug; see https://github.com/WebAssembly/spec/pull/420 .

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +748,5 @@
>          else
>              masm.convertInt64ToDouble(input, output);
>      } else {
>          if (lir->mir()->isUnsigned())
> +            masm.convertUInt64ToFloat32(input, output, ToRegister(lir->getTemp(0)));

This temp is consequently unneeded.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3073,5 @@
>      bool convertI64ToFloatNeedsTemp(bool isUnsigned) const {
>  # if defined(JS_CODEGEN_X86)
>          return isUnsigned && AssemblerX86Shared::HasSSE3();
>  # else
> +        return isUnsigned;

This code could also be reorganized to return false for the f32 case, though it's not urgent, so adding a comment about this possibility for now would also be ok.
Attachment #8834068 - Flags: review?(sunfish) → review+
I was mistaken about f32.convert_u/i64. It does need an adjustment.
(In reply to Dan Gohman [:sunfish] from comment #7)
> I was mistaken about f32.convert_u/i64. It does need an adjustment.

Fun, I was holding on pushing my patch because you said that. It appears that awsm also found test cases for that \o/.
So, let's go down the rabbit hole...

Consider the following test case:

(module    
    (func (export "func_0") (result i32)
        i64.const 0x100404900000008
        f32.convert_u/i64
        f32.const 72128280609685500
        f32.eq
    ) ;; found by awsm

    (func (export "func_1") (result i32)
        i64.const 0x7fffff4000000001
        f32.convert_u/i64
        f32.const 0x1.fffffep+62
        f32.eq
    )

    (func (export "func_2") (result i32)
        i64.const 0x8000008000000001
        f32.convert_u/i64
        f32.const 0x1.000002p+63
        f32.eq
    ) ;; last two from your Github issue
)

Now, on x86 (32 bits), the three tests fail with SSE3, as well as with --no-SSE3 (that is only SSE2, using the x87 FPU instructions). In this configuration, f32.convert_u/i64 is actually an uint64 -> f64 -> f32 conversion. Messing with the precision mode of the FPU control word doesn't change anything (more on that below).

If I change convertUInt64ToFloat32 to just do the same thing as SSE2 for doubles, but tweaking the final fstp to an fstps (for a single precision st->xmm move), then the two first tests pass, but not the third one! The error is just on the LSB of the 64-bits word returned by the final fstp (as shown by gdb, `info float`), which is 1 in the correct case, and 0 in spidermonkey.

Comparing with the output of gcc/clang for the same conversion, we have the exact same code, so what's going on? The only difference is that the FPU control word is set to use 80 bits doubles (extended precision) in a regular C++ program, and we do set this word to use 64 bits (double precision) doubles in Spidermonkey [1]. If I change our code to use extended precision as well in the FPU, all the three tests pass; that might not be compatible with the rest of the engine though (I seem to remember we've been setting it to avoid differential testing issues, where C++ code was using the x87 FPU and the JIT were using the xmm instructions for an equivalent arithmetic operation).

I'll do a try-build to see if anything is broken by the control word being set to extended precision.

Other alternatives:
- maybe we can use extended precision in general in Spidermonkey, now that we need to compile with at least SSE2 on x86. https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a719bf4f193efde7c2f7b51da35441a92c2556
- v8 uses a callout here, but I assume the FPU control word issue may arise in this case too.
- set the FPU control word in a local scope (which could be either the macro-instruction scope, or when entering a wasm module (numerous caveats when calling into FFI, etc.)), reset it when leaving the scope.

[1] http://searchfox.org/mozilla-central/source/js/src/jsnum.cpp#1145
Wow. Yeah, if we can safely change back to extended precision, that sounds like the nicest option. It'd be nice if we don't have to call FIX_FPU at the beginning of every thread that might do floating point. And, it would seem that switching to extended precision mode would fix incorrect rounding in C++ or Rust code in Firefox compiled by gcc or LLVM, for uint64_t->float conversions or in theory other things too.

As another option, it looks like there's a sequence for i386 here:

https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/i386/floatundisf.S

which avoids using the x87 entirely (see the "branch-free, x87-free implementation" version).
Priority: -- → P1
Attached patch wip.patch (obsolete) — Splinter Review
Almost done: just missing new ARM callouts for u64 -> f32.
Attachment #8834068 - Attachment is obsolete: true
Depends on: 1342176
Attached patch wip.patch (obsolete) — Splinter Review
Fixes uint64 -> FP conversions on the ARM simulator too; a try-run will confirm this also works on real devices.

Of courses, there are also issues with plain int64 -> FP conversions I need to investigate.
Attachment #8840532 - Attachment is obsolete: true
Attachment #8840824 - Attachment is obsolete: true
Attachment #8840877 - Flags: review?(sunfish)
Comment on attachment 8840877 [details] [diff] [review]
conversions.patch

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

Looks good. And I just added a few more tests to https://github.com/WebAssembly/spec/issues/421 which would be good to verify as well.
Attachment #8840877 - Flags: review?(sunfish) → review+
For what it's worth, I've added the new test cases, and checked that they pass on x86 and x64. I've also managed to cross-compile the shell to my ARM based phone, and confirmed that all the tests pass there too.

A few Windows-only failures have crop up in my try run; I wonder if it's the precision issue happening again...
in addition to the previous patch: on x86 u64 -> f32 path, set and restore the FPU state. This might get added to more integer to floating-point conversion paths, if necessary and as shown by fuzzing.
Attachment #8842795 - Flags: review?(sunfish)
Comment on attachment 8842795 [details] [diff] [review]
set-restore.patch

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

Looks good.
Attachment #8842795 - Flags: review?(sunfish) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/654820d0aed7
Fix uint64 to floating-point conversion; r=sunfish
https://hg.mozilla.org/integration/mozilla-inbound/rev/e824e868c379
Set and restore FPU precision before applying the u64 -> f32 conversion on x86; r=sunfish
Thanks for the review.

Since these changes are a bit invasive and we're not even sure we won't need more (only fuzzing might say), I'd be tempted to let this ride the trains, especially since this should impact correctness only on edge cases. If anybody thinks otherwise and has compelling arguments, I'll request uplift.
https://hg.mozilla.org/mozilla-central/rev/654820d0aed7
https://hg.mozilla.org/mozilla-central/rev/e824e868c379
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Since these were no objections and all of these are edge cases, I don't think it's worth uplifting. Clearing flags and setting wontfix on other platforms.
You need to log in before you can comment on or make changes to this bug.