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.

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).

(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.

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.

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.

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 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 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.

Pushed by bbouvier@mozilla.com: 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

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