Negative zero checks cause unnecessary speculation failures on SunSpider on ARMv7...

[Title] Negative zero checks cause unnecessary speculation failures on SunSpider on ARMv7.
[Issue#] N/A
[Problem] While testing SunSpider math-spectral-norm on x86-64 and ARMv7 Linux,
          there are a lot of "Speculation failures" on ARM, but not on x86.
[Cause] N/A
[Solution] Check nodeCanIgnoreNegativeZero() for DoubleAsInt32 nodes, and create zero-check only when needed.

Change-Id: I50384d628a7ed8928d1378c52fe100f6de02197d

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerARM.h b/Source/JavaScriptCore/assembler/MacroAssemblerARM.h
index bb71f71..42efcdd 100644 (file)
--- a/Source/JavaScriptCore/assembler/MacroAssemblerARM.h
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerARM.h
@@ -1190,7 +1190,7 @@ public:
     // If the result is not representable as a 32 bit value, branch.
     // May also branch for some values that are representable in 32 bits
     // (specifically, in this case, 0).
-    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
+    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID, bool negZeroCheck = true)
     {
         m_assembler.vcvt_s32_f64_r(ARMRegisters::SD0 << 1, src);
         m_assembler.vmov_arm32_r(dest, ARMRegisters::SD0 << 1);
@@ -1200,7 +1200,8 @@ public:
         failureCases.append(branchDouble(DoubleNotEqualOrUnordered, src, ARMRegisters::SD0));
 
         // If the result is zero, it might have been -0.0, and 0.0 equals to -0.0
-        failureCases.append(branchTest32(Zero, dest));
+        if (negZeroCheck)
+            failureCases.append(branchTest32(Zero, dest));
     }
 
     Jump branchDoubleNonZero(FPRegisterID reg, FPRegisterID scratch)

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h b/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h
index 183e8f9..50d4216 100644 (file)
--- a/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h
@@ -1063,7 +1063,7 @@ public:
     // If the result is not representable as a 32 bit value, branch.
     // May also branch for some values that are representable in 32 bits
     // (specifically, in this case, 0).
-    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID)
+    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID, bool negZeroCheck = true)
     {
         m_assembler.vcvt_floatingPointToSigned(fpTempRegisterAsSingle(), src);
         m_assembler.vmov(dest, fpTempRegisterAsSingle());
@@ -1073,7 +1073,8 @@ public:
         failureCases.append(branchDouble(DoubleNotEqualOrUnordered, src, fpTempRegister));
 
         // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
-        failureCases.append(branchTest32(Zero, dest));
+        if (negZeroCheck)
+            failureCases.append(branchTest32(Zero, dest));
     }
 
     Jump branchDoubleNonZero(FPRegisterID reg, FPRegisterID)

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h b/Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h
index 8b3ce9f..0f457b6 100644 (file)
--- a/Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h
@@ -1853,13 +1853,14 @@ public:
     // If the result is not representable as a 32 bit value, branch.
     // May also branch for some values that are representable in 32 bits
     // (specifically, in this case, 0).
-    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
+    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
     {
         m_assembler.cvtwd(fpTempRegister, src);
         m_assembler.mfc1(dest, fpTempRegister);
 
         // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
-        failureCases.append(branch32(Equal, dest, MIPSRegisters::zero));
+        if (negZeroCheck)
+            failureCases.append(branch32(Equal, dest, MIPSRegisters::zero));
 
         // Convert the integer result back to float & compare to the original value - if not equal or unordered (NaN) then jump.
         convertInt32ToDouble(dest, fpTemp);

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerSH4.h b/Source/JavaScriptCore/assembler/MacroAssemblerSH4.h
index badf35f..1e4070b 100644 (file)
--- a/Source/JavaScriptCore/assembler/MacroAssemblerSH4.h
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerSH4.h
@@ -1821,20 +1821,22 @@ void or32(TrustedImm32 imm, RegisterID src, RegisterID dest)
         return branchTrue();
     }
 
-    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
+    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
     {
         m_assembler.ftrcdrmfpul(src);
         m_assembler.stsfpulReg(dest);
         convertInt32ToDouble(dest, fscratch);
         failureCases.append(branchDouble(DoubleNotEqualOrUnordered, fscratch, src));
 
-        if (dest == SH4Registers::r0)
-            m_assembler.cmpEqImmR0(0, dest);
-        else {
-            m_assembler.movImm8(0, scratchReg3);
-            m_assembler.cmplRegReg(scratchReg3, dest, SH4Condition(Equal));
+        if (negZeroCheck) {
+            if (dest == SH4Registers::r0)
+                m_assembler.cmpEqImmR0(0, dest);
+            else {
+                m_assembler.movImm8(0, scratchReg3);
+                m_assembler.cmplRegReg(scratchReg3, dest, SH4Condition(Equal));
+            }
+            failureCases.append(branchTrue());
         }
-        failureCases.append(branchTrue());
     }
 
     void neg32(RegisterID dst)

diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h b/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h
index 115b337..8679eff 100644 (file)
--- a/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h
@@ -873,13 +873,14 @@ public:
     // If the result is not representable as a 32 bit value, branch.
     // May also branch for some values that are representable in 32 bits
     // (specifically, in this case, 0).
-    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
+    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
     {
         ASSERT(isSSE2Present());
         m_assembler.cvttsd2si_rr(src, dest);
 
         // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
-        failureCases.append(branchTest32(Zero, dest));
+        if (negZeroCheck)
+            failureCases.append(branchTest32(Zero, dest));
 
         // Convert the integer result back to float & compare to the original value - if not equal or unordered (NaN) then jump.
         convertInt32ToDouble(dest, fpTemp);

diff --git a/Source/JavaScriptCore/dfg/DFGNode.h b/Source/JavaScriptCore/dfg/DFGNode.h
index 60cdd4b..1796815 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGNode.h
+++ b/Source/JavaScriptCore/dfg/DFGNode.h
@@ -394,7 +394,7 @@ struct Node {
     NodeFlags arithNodeFlags()
     {
         NodeFlags result = m_flags;
-        if (op() == ArithMul || op() == ArithDiv || op() == ArithMod)
+        if (op() == ArithMul || op() == ArithDiv || op() == ArithMod || op() == ArithNegate || op() == DoubleAsInt32)
             return result;
         return result & ~NodeNeedsNegZero;
     }

diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index 94f69ab..21461c6 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -107,14 +107,15 @@ private:
         if (!m_graph.isNumberConstant(nodeIndex))
             return false;
         double value = m_graph.valueOfNumberConstant(nodeIndex);
-        return !value && 1.0 / value < 0.0;
+        return (value || 1.0 / value > 0.0);
     }
     
-    bool isNotZero(NodeIndex nodeIndex)
+    bool isNotPosZero(NodeIndex nodeIndex)
     {
         if (!m_graph.isNumberConstant(nodeIndex))
             return false;
-        return !!m_graph.valueOfNumberConstant(nodeIndex);
+        double value = m_graph.valueOfNumberConstant(nodeIndex);
+        return (value || 1.0 / value < 0.0);
     }
     
     void propagate(Node& node)
@@ -294,7 +295,7 @@ private:
                     changed |= mergePrediction(SpecDouble);
             }
 
-            if (isNotZero(node.child1().index()) || isNotZero(node.child2().index()))
+            if (isNotNegZero(node.child1().index()) || isNotPosZero(node.child2().index()))
                 flags &= ~NodeNeedsNegZero;
             
             changed |= m_graph[node.child1()].mergeFlags(flags);

diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
index 09e9534..abe4734 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -1851,7 +1851,8 @@ void SpeculativeJIT::compileDoubleAsInt32(Node& node)
     GPRReg resultGPR = result.gpr();
 
     JITCompiler::JumpList failureCases;
-    m_jit.branchConvertDoubleToInt32(valueFPR, resultGPR, failureCases, scratchFPR);
+    bool negZeroCheck = !nodeCanIgnoreNegativeZero(node.arithNodeFlags());
+    m_jit.branchConvertDoubleToInt32(valueFPR, resultGPR, failureCases, scratchFPR, negZeroCheck);
     forwardSpeculationCheck(Overflow, JSValueRegs(), NoNode, failureCases, ValueRecovery::inFPR(valueFPR));
 
     integerResult(resultGPR, m_compileIndex);
@@ -2384,12 +2385,12 @@ void SpeculativeJIT::compileSoftModulo(Node& node)
                 speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branch32(JITCompiler::Equal, eax.gpr(), TrustedImm32(-2147483647-1)));
             m_jit.assembler().cdq();
             m_jit.assembler().idivl_r(scratchGPR);
-            // Check that we're not about to create negative zero.
-            // FIXME: if the node use doesn't care about neg zero, we can do this more easily.
-            JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
-            speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
-            numeratorPositive.link(&m_jit);
-            
+            if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
+                // Check that we're not about to create negative zero.
+                JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
+                speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
+                numeratorPositive.link(&m_jit);
+            }
             if (op1SaveGPR != op1Gpr)
                 unlock(op1SaveGPR);
 
@@ -2397,6 +2398,33 @@ void SpeculativeJIT::compileSoftModulo(Node& node)
             return;
         }
     }
+#elif CPU(ARM_THUMB2)
+    if (isInt32Constant(node.child2().index())) {
+        int32_t divisor = valueOfInt32Constant(node.child2().index());
+        if (divisor > 0 && !(divisor & (divisor - 1))) { // If power of 2 then just mask
+            GPRReg dividendGPR = op1.gpr();
+            GPRTemporary result(this);
+            GPRReg resultGPR = result.gpr();
+
+            m_jit.assembler().cmp(dividendGPR, ARMThumbImmediate::makeEncodedImm(0));
+            m_jit.assembler().it(ARMv7Assembler::ConditionLT, false);
+            m_jit.assembler().neg(resultGPR, dividendGPR);
+            m_jit.assembler().mov(resultGPR, dividendGPR);
+            m_jit.and32(TrustedImm32(divisor - 1), resultGPR);
+            m_jit.assembler().it(ARMv7Assembler::ConditionLT);
+            m_jit.assembler().neg(resultGPR, resultGPR);
+
+            if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
+                // Check that we're not about to create negative zero.
+                JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, dividendGPR, TrustedImm32(0));
+                speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, resultGPR));
+                numeratorPositive.link(&m_jit);
+            }
+
+            integerResult(resultGPR, m_compileIndex);
+            return;
+        }
+    }
 #endif
 
     SpeculateIntegerOperand op2(this, node.child2());
@@ -2456,11 +2484,12 @@ void SpeculativeJIT::compileSoftModulo(Node& node)
     if (op2TempGPR != InvalidGPRReg)
         unlock(op2TempGPR);
 
-    // Check that we're not about to create negative zero.
-    // FIXME: if the node use doesn't care about neg zero, we can do this more easily.
-    JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
-    speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
-    numeratorPositive.link(&m_jit);
+    if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
+        // Check that we're not about to create negative zero.
+        JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
+        speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
+        numeratorPositive.link(&m_jit);
+    }
     
     if (op1SaveGPR != op1GPR)
         unlock(op1SaveGPR);
@@ -2480,8 +2509,14 @@ void SpeculativeJIT::compileSoftModulo(Node& node)
     FPRTemporary scratch(this);
     GPRTemporary intResult(this);
     JITCompiler::JumpList failureCases;
-    m_jit.branchConvertDoubleToInt32(result.fpr(), intResult.gpr(), failureCases, scratch.fpr());
+    m_jit.branchConvertDoubleToInt32(result.fpr(), intResult.gpr(), failureCases, scratch.fpr(), false);
     speculationCheck(Overflow, JSValueRegs(), NoNode, failureCases);
+    if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
+        // Check that we're not about to create negative zero.
+        JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1GPR, TrustedImm32(0));
+        speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, intResult.gpr()));
+        numeratorPositive.link(&m_jit);
+    }
     
     integerResult(intResult.gpr(), m_compileIndex);
 #endif // CPU(X86) || CPU(X86_64)
@@ -2649,7 +2684,7 @@ void SpeculativeJIT::compileArithNegate(Node& node)
         else {
             speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchNeg32(MacroAssembler::Overflow, result.gpr()));
             if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags()))
-                speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(MacroAssembler::Zero, result.gpr()));
+                speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branchTest32(MacroAssembler::Zero, result.gpr()));
         }
 
         integerResult(result.gpr(), m_compileIndex);