From d79e90a16366ac847fdd7929eef4afa40f62d7c2 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 23 Apr 2015 12:12:41 +0200 Subject: [PATCH] Bug 1142668: Fix int32x4 to float32x4 conversions in the JIT; r=sunfish --HG-- extra : rebase_source : 977a3f5335d0044e9fb15b04a16baa58a0f1855a --- js/src/asmjs/AsmJSValidate.cpp | 1 + js/src/jit-test/tests/SIMD/convert.js | 38 +++++++++++++- js/src/jit/IonTypes.h | 14 ++++- js/src/jit/LIR-Common.h | 11 +++- js/src/jit/Lowering.cpp | 8 +-- js/src/jit/MIR.cpp | 2 +- js/src/jit/MIR.h | 7 ++- js/src/jit/MIRGenerator.h | 13 ++++- js/src/jit/MIRGraph.cpp | 5 +- .../x86-shared/CodeGenerator-x86-shared.cpp | 51 +++++++++++++++++++ .../jit/x86-shared/CodeGenerator-x86-shared.h | 23 +++++++++ .../x86-shared/MacroAssembler-x86-shared.h | 5 +- 12 files changed, 164 insertions(+), 14 deletions(-) diff --git a/js/src/asmjs/AsmJSValidate.cpp b/js/src/asmjs/AsmJSValidate.cpp index ac471c8f079d..7e695e5b1185 100644 --- a/js/src/asmjs/AsmJSValidate.cpp +++ b/js/src/asmjs/AsmJSValidate.cpp @@ -2525,6 +2525,7 @@ class FunctionCompiler options, alloc_, graph_, info_, optimizationInfo, &m().onOutOfBoundsLabel(), + /* conversionErrorLabel = */ nullptr, m().usesSignalHandlersForOOB()); if (!newBlock(/* pred = */ nullptr, &curBlock_, fn_)) diff --git a/js/src/jit-test/tests/SIMD/convert.js b/js/src/jit-test/tests/SIMD/convert.js index aab00d31e57c..d127a60c1d89 100644 --- a/js/src/jit-test/tests/SIMD/convert.js +++ b/js/src/jit-test/tests/SIMD/convert.js @@ -1,6 +1,6 @@ load(libdir + 'simd.js'); -setJitCompilerOption("ion.warmup.trigger", 50); +setJitCompilerOption("ion.warmup.trigger", 30); var cast = (function() { var i32 = new Int32Array(1); @@ -19,6 +19,7 @@ var cast = (function() { })(); function f() { + // No bailout here. var f4 = SIMD.float32x4(1, 2, 3, 4); var i4 = SIMD.int32x4(1, 2, 3, 4); var BitOrZero = (x) => x | 0; @@ -30,5 +31,40 @@ function f() { } } +function uglyDuckling(val) { + // We bail out when i == 149 because the conversion will return + // 0x80000000 and the input actually wasn't in bounds. + print('entering uglyDuckling'); + val = Math.fround(val); + for (var i = 0; i < 150; i++) { + var caught = false; + try { + var v = SIMD.float32x4(i < 149 ? 0 : val, 0, 0, 0) + SIMD.int32x4.fromFloat32x4(v); + } catch(e) { + assertEq(e instanceof RangeError, true); + assertEq(i, 149); + caught = true; + } + assertEq(i < 149 || caught, true); + } +} + +function dontBail() { + print('entering dontbail'); + // On x86, the conversion will return 0x80000000, which will imply that we + // check the input values. However, we shouldn't bail out in this case. + for (var i = 0; i < 150; i++) { + var v = SIMD.float32x4(i < 149 ? 0 : -Math.pow(2, 31), 0, 0, 0) + SIMD.int32x4.fromFloat32x4(v); + } +} + f(); +dontBail(); +dontBail(); + +uglyDuckling(Math.pow(2, 31)); +uglyDuckling(NaN); +uglyDuckling(-Math.pow(2, 32)); diff --git a/js/src/jit/IonTypes.h b/js/src/jit/IonTypes.h index 0f47a85ad9aa..77ed06a32823 100644 --- a/js/src/jit/IonTypes.h +++ b/js/src/jit/IonTypes.h @@ -545,7 +545,19 @@ static inline bool IsSimdType(MIRType type) { return type == MIRType_Int32x4 || type == MIRType_Float32x4; -}; +} + +static inline bool +IsFloatingPointSimdType(MIRType type) +{ + return type == MIRType_Float32x4; +} + +static inline bool +IsIntegerSimdType(MIRType type) +{ + return type == MIRType_Int32x4; +} static inline bool IsMagicType(MIRType type) diff --git a/js/src/jit/LIR-Common.h b/js/src/jit/LIR-Common.h index ef5103d23105..45672b41ac97 100644 --- a/js/src/jit/LIR-Common.h +++ b/js/src/jit/LIR-Common.h @@ -3762,12 +3762,19 @@ class LInt32x4ToFloat32x4 : public LInstructionHelper<1, 1, 0> } }; -class LFloat32x4ToInt32x4 : public LInstructionHelper<1, 1, 0> +class LFloat32x4ToInt32x4 : public LInstructionHelper<1, 1, 1> { public: LIR_HEADER(Float32x4ToInt32x4); - explicit LFloat32x4ToInt32x4(const LAllocation& input) { + explicit LFloat32x4ToInt32x4(const LAllocation& input, const LDefinition& temp) { setOperand(0, input); + setTemp(0, temp); + } + const LDefinition* temp() { + return getTemp(0); + } + const MSimdConvert* mir() const { + return mir_->toSimdConvert(); } }; diff --git a/js/src/jit/Lowering.cpp b/js/src/jit/Lowering.cpp index c280d3db25e0..02c704e7af67 100644 --- a/js/src/jit/Lowering.cpp +++ b/js/src/jit/Lowering.cpp @@ -3890,11 +3890,13 @@ LIRGenerator::visitSimdConvert(MSimdConvert* ins) { MOZ_ASSERT(IsSimdType(ins->type())); MDefinition* input = ins->input(); - LUse use = useRegisterAtStart(input); - + LUse use = useRegister(input); if (ins->type() == MIRType_Int32x4) { MOZ_ASSERT(input->type() == MIRType_Float32x4); - define(new(alloc()) LFloat32x4ToInt32x4(use), ins); + LFloat32x4ToInt32x4* lir = new(alloc()) LFloat32x4ToInt32x4(use, temp()); + if (!gen->conversionErrorLabel()) + assignSnapshot(lir, Bailout_BoundsCheck); + define(lir, ins); } else if (ins->type() == MIRType_Float32x4) { MOZ_ASSERT(input->type() == MIRType_Int32x4); define(new(alloc()) LInt32x4ToFloat32x4(use), ins); diff --git a/js/src/jit/MIR.cpp b/js/src/jit/MIR.cpp index ea874f7df06a..da6a4e0651ab 100644 --- a/js/src/jit/MIR.cpp +++ b/js/src/jit/MIR.cpp @@ -1018,7 +1018,7 @@ MSimdBinaryComp::printOpcode(FILE* fp) const PrintOpcodeOperation(this, fp); } void -MSimdShift::printOpcode(FILE *fp) const +MSimdShift::printOpcode(FILE* fp) const { PrintOpcodeOperation(this, fp); } diff --git a/js/src/jit/MIR.h b/js/src/jit/MIR.h index fa1c05f962e6..b4fffc5860a9 100644 --- a/js/src/jit/MIR.h +++ b/js/src/jit/MIR.h @@ -1531,9 +1531,14 @@ class MSimdConvert : MUnaryInstruction(obj) { MOZ_ASSERT(IsSimdType(toType)); - setMovable(); setResultType(toType); specialization_ = fromType; // expects fromType as input + + setMovable(); + if (IsFloatingPointSimdType(fromType) && IsIntegerSimdType(toType)) { + // Does the extra range check => do not remove + setGuard(); + } } public: diff --git a/js/src/jit/MIRGenerator.h b/js/src/jit/MIRGenerator.h index 05e86b9f762a..30fcab97eb43 100644 --- a/js/src/jit/MIRGenerator.h +++ b/js/src/jit/MIRGenerator.h @@ -38,7 +38,9 @@ class MIRGenerator MIRGenerator(CompileCompartment* compartment, const JitCompileOptions& options, TempAllocator* alloc, MIRGraph* graph, CompileInfo* info, const OptimizationInfo* optimizationInfo, - Label* outOfBoundsLabel = nullptr, bool usesSignalHandlersForAsmJSOOB = false); + Label* outOfBoundsLabel = nullptr, + Label* conversionErrorLabel = nullptr, + bool usesSignalHandlersForAsmJSOOB = false); TempAllocator& alloc() { return *alloc_; @@ -202,6 +204,10 @@ class MIRGenerator void addAbortedPreliminaryGroup(ObjectGroup* group); Label* outOfBoundsLabel_; + // Label where we should jump in asm.js mode, in the case where we have an + // invalid conversion or a loss of precision (when converting from a + // floating point SIMD type into an integer SIMD type). + Label* conversionErrorLabel_; #if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB) bool usesSignalHandlersForAsmJSOOB_; #endif @@ -229,7 +235,12 @@ class MIRGenerator return nurseryObjects_; } + Label* conversionErrorLabel() const { + MOZ_ASSERT((conversionErrorLabel_ != nullptr) == compilingAsmJS()); + return conversionErrorLabel_; + } Label* outOfBoundsLabel() const { + MOZ_ASSERT(compilingAsmJS()); return outOfBoundsLabel_; } bool needsAsmJSBoundsCheckBranch(const MAsmJSHeapAccess* access) const; diff --git a/js/src/jit/MIRGraph.cpp b/js/src/jit/MIRGraph.cpp index 52e5ee7f758c..6e6ae4b7d9eb 100644 --- a/js/src/jit/MIRGraph.cpp +++ b/js/src/jit/MIRGraph.cpp @@ -20,7 +20,9 @@ using mozilla::Swap; MIRGenerator::MIRGenerator(CompileCompartment* compartment, const JitCompileOptions& options, TempAllocator* alloc, MIRGraph* graph, CompileInfo* info, const OptimizationInfo* optimizationInfo, - Label* outOfBoundsLabel, bool usesSignalHandlersForAsmJSOOB) + Label* outOfBoundsLabel, + Label* conversionErrorLabel, + bool usesSignalHandlersForAsmJSOOB) : compartment(compartment), info_(info), optimizationInfo_(optimizationInfo), @@ -42,6 +44,7 @@ MIRGenerator::MIRGenerator(CompileCompartment* compartment, const JitCompileOpti instrumentedProfilingIsCached_(false), nurseryObjects_(*alloc), outOfBoundsLabel_(outOfBoundsLabel), + conversionErrorLabel_(conversionErrorLabel), #if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB) usesSignalHandlersForAsmJSOOB_(usesSignalHandlersForAsmJSOOB), #endif diff --git a/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp b/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp index df45d7c34481..654b6d4375a2 100644 --- a/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp +++ b/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp @@ -2175,7 +2175,58 @@ CodeGeneratorX86Shared::visitFloat32x4ToInt32x4(LFloat32x4ToInt32x4* ins) { FloatRegister in = ToFloatRegister(ins->input()); FloatRegister out = ToFloatRegister(ins->output()); + Register temp = ToRegister(ins->temp()); + masm.convertFloat32x4ToInt32x4(in, out); + + OutOfLineSimdFloatToIntCheck *ool = new(alloc()) OutOfLineSimdFloatToIntCheck(temp, in, ins); + addOutOfLineCode(ool, ins->mir()); + + static const SimdConstant InvalidResult = SimdConstant::SplatX4(int32_t(-2147483648)); + + masm.loadConstantInt32x4(InvalidResult, ScratchSimdReg); + masm.packedEqualInt32x4(Operand(out), ScratchSimdReg); + // TODO (bug 1156228): If we have SSE4.1, we can use PTEST here instead of + // the two following instructions. + masm.vmovmskps(ScratchSimdReg, temp); + masm.cmp32(temp, Imm32(0)); + masm.j(Assembler::NotEqual, ool->entry()); + + masm.bind(ool->rejoin()); +} + +void +CodeGeneratorX86Shared::visitOutOfLineSimdFloatToIntCheck(OutOfLineSimdFloatToIntCheck *ool) +{ + static const SimdConstant Int32MaxX4 = SimdConstant::SplatX4(2147483647.f); + static const SimdConstant Int32MinX4 = SimdConstant::SplatX4(-2147483648.f); + + Label bail; + Label* onConversionError = gen->conversionErrorLabel(); + if (!onConversionError) + onConversionError = &bail; + + FloatRegister input = ool->input(); + Register temp = ool->temp(); + + masm.loadConstantFloat32x4(Int32MinX4, ScratchSimdReg); + masm.vcmpleps(Operand(input), ScratchSimdReg, ScratchSimdReg); + masm.vmovmskps(ScratchSimdReg, temp); + masm.cmp32(temp, Imm32(15)); + masm.j(Assembler::NotEqual, onConversionError); + + masm.loadConstantFloat32x4(Int32MaxX4, ScratchSimdReg); + masm.vcmpleps(Operand(input), ScratchSimdReg, ScratchSimdReg); + masm.vmovmskps(ScratchSimdReg, temp); + masm.cmp32(temp, Imm32(0)); + masm.j(Assembler::NotEqual, onConversionError); + + masm.jump(ool->rejoin()); + + if (bail.used()) { + masm.bind(&bail); + bailout(ool->ins()->snapshot()); + } } void diff --git a/js/src/jit/x86-shared/CodeGenerator-x86-shared.h b/js/src/jit/x86-shared/CodeGenerator-x86-shared.h index 1c3f610f6531..08fe1a048dbd 100644 --- a/js/src/jit/x86-shared/CodeGenerator-x86-shared.h +++ b/js/src/jit/x86-shared/CodeGenerator-x86-shared.h @@ -69,6 +69,28 @@ class CodeGeneratorX86Shared : public CodeGeneratorShared } }; + // Additional bounds check for vector Float to Int conversion, when the + // undefined pattern is seen. Might imply a bailout. + class OutOfLineSimdFloatToIntCheck : public OutOfLineCodeBase + { + Register temp_; + FloatRegister input_; + LInstruction* ins_; + + public: + OutOfLineSimdFloatToIntCheck(Register temp, FloatRegister input, LInstruction *ins) + : temp_(temp), input_(input), ins_(ins) + {} + + Register temp() const { return temp_; } + FloatRegister input() const { return input_; } + LInstruction* ins() const { return ins_; } + + void accept(CodeGeneratorX86Shared* codegen) { + codegen->visitOutOfLineSimdFloatToIntCheck(this); + } + }; + // Functions for emitting bounds-checking code with branches. MOZ_WARN_UNUSED_RESULT uint32_t emitAsmJSBoundsCheckBranch(const MAsmJSHeapAccess* mir, const MInstruction* ins, @@ -282,6 +304,7 @@ class CodeGeneratorX86Shared : public CodeGeneratorShared void visitModOverflowCheck(ModOverflowCheck* ool); void visitReturnZero(ReturnZero* ool); void visitOutOfLineTableSwitch(OutOfLineTableSwitch* ool); + void visitOutOfLineSimdFloatToIntCheck(OutOfLineSimdFloatToIntCheck* ool); void generateInvalidateEpilogue(); }; diff --git a/js/src/jit/x86-shared/MacroAssembler-x86-shared.h b/js/src/jit/x86-shared/MacroAssembler-x86-shared.h index 8c96067dcb3e..d6b57ad0057a 100644 --- a/js/src/jit/x86-shared/MacroAssembler-x86-shared.h +++ b/js/src/jit/x86-shared/MacroAssembler-x86-shared.h @@ -934,11 +934,10 @@ class MacroAssemblerX86Shared : public Assembler } void convertFloat32x4ToInt32x4(FloatRegister src, FloatRegister dest) { - // TODO: Note that if the conversion failed (because the converted + // Note that if the conversion failed (because the converted // result is larger than the maximum signed int32, or less than the // least signed int32, or NaN), this will return the undefined integer - // value (0x8000000). Spec should define what to do in such cases. See - // also bug 1068020. + // value (0x8000000). vcvttps2dq(src, dest); } void convertInt32x4ToFloat32x4(FloatRegister src, FloatRegister dest) {