Closed
Bug 1336139
Opened 8 years ago
Closed 8 years ago
wasm: differential testing issue with f64.convert_u/i64
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
Comment hidden (obsolete) |
Assignee | ||
Comment 1•8 years ago
|
||
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
*/
Assignee | ||
Comment 2•8 years ago
|
||
Reduced to:
i64.const 13800889852755076097
f64.convert_u/i64
Summary: wasm: differential testing issue → wasm: differential testing issue with f64.convert_u/i64
Assignee | ||
Comment 3•8 years ago
|
||
[Tracking Requested - why for this release]: incorrect behavior in wasm
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
status-firefox52:
--- → affected
status-firefox53:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
I was mistaken about f32.convert_u/i64. It does need an adjustment.
Assignee | ||
Comment 8•8 years ago
|
||
(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/.
Assignee | ||
Comment 9•8 years ago
|
||
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
Comment 10•8 years ago
|
||
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).
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 11•8 years ago
|
||
Almost done: just missing new ARM callouts for u64 -> f32.
Attachment #8834068 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8840824 -
Attachment is obsolete: true
Attachment #8840877 -
Flags: review?(sunfish)
Comment 14•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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...
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Comment 19•8 years ago
|
||
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
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/654820d0aed7
https://hg.mozilla.org/mozilla-central/rev/e824e868c379
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 22•8 years ago
|
||
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.
tracking-firefox52:
+ → ---
tracking-firefox53:
+ → ---
You need to log in
before you can comment on or make changes to this bug.