Title: [111355] trunk/Source/_javascript_Core
Revision
111355
Author
[email protected]
Date
2012-03-19 22:15:50 -0700 (Mon, 19 Mar 2012)

Log Message

Division optimizations fail to infer cases of truncated division and
mishandle -2147483648/-1
https://bugs.webkit.org/show_bug.cgi?id=81428
<rdar://problem/11067382>

Reviewed by Oliver Hunt.

If you're a division over integers and you're only used as an integer, then you're
an integer division and remainder checks become unnecessary. If you're dividing
-2147483648 by -1, don't crash.

* assembler/MacroAssemblerX86Common.h:
(MacroAssemblerX86Common):
(JSC::MacroAssemblerX86Common::add32):
* dfg/DFGSpeculativeJIT.cpp:
(DFG):
(JSC::DFG::SpeculativeJIT::compileIntegerArithDivForX86):
* dfg/DFGSpeculativeJIT.h:
(SpeculativeJIT):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* llint/LowLevelInterpreter64.asm:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (111354 => 111355)


--- trunk/Source/_javascript_Core/ChangeLog	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-03-20 05:15:50 UTC (rev 111355)
@@ -1,3 +1,30 @@
+2012-03-19  Filip Pizlo  <[email protected]>
+
+        Division optimizations fail to infer cases of truncated division and
+        mishandle -2147483648/-1
+        https://bugs.webkit.org/show_bug.cgi?id=81428
+        <rdar://problem/11067382>
+
+        Reviewed by Oliver Hunt.
+
+        If you're a division over integers and you're only used as an integer, then you're
+        an integer division and remainder checks become unnecessary. If you're dividing
+        -2147483648 by -1, don't crash.
+
+        * assembler/MacroAssemblerX86Common.h:
+        (MacroAssemblerX86Common):
+        (JSC::MacroAssemblerX86Common::add32):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (DFG):
+        (JSC::DFG::SpeculativeJIT::compileIntegerArithDivForX86):
+        * dfg/DFGSpeculativeJIT.h:
+        (SpeculativeJIT):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * llint/LowLevelInterpreter64.asm:
+
 2012-03-19  Benjamin Poulain  <[email protected]>
 
         Simplify SmallStrings

Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h (111354 => 111355)


--- trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerX86Common.h	2012-03-20 05:15:50 UTC (rev 111355)
@@ -129,6 +129,11 @@
     {
         m_assembler.addl_rm(src, dest.offset, dest.base);
     }
+
+    void add32(TrustedImm32 imm, RegisterID src, RegisterID dest)
+    {
+        m_assembler.leal_mr(imm.m_value, src, dest);
+    }
     
     void and32(RegisterID src, RegisterID dest)
     {

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (111354 => 111355)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-03-20 05:15:50 UTC (rev 111355)
@@ -2450,6 +2450,82 @@
     doubleResult(result.fpr(), m_compileIndex);
 }
 
+#if CPU(X86) || CPU(X86_64)
+void SpeculativeJIT::compileIntegerArithDivForX86(Node& node)
+{
+    SpeculateIntegerOperand op1(this, node.child1());
+    SpeculateIntegerOperand op2(this, node.child2());
+    GPRTemporary eax(this, X86Registers::eax);
+    GPRTemporary edx(this, X86Registers::edx);
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.gpr();
+    
+    GPRReg op2TempGPR;
+    GPRReg temp;
+    if (op2GPR == X86Registers::eax || op2GPR == X86Registers::edx) {
+        op2TempGPR = allocate();
+        temp = op2TempGPR;
+    } else {
+        op2TempGPR = InvalidGPRReg;
+        if (op1GPR == X86Registers::eax)
+            temp = X86Registers::edx;
+        else
+            temp = X86Registers::eax;
+    }
+    
+    ASSERT(temp != op1GPR);
+    ASSERT(temp != op2GPR);
+    
+    m_jit.add32(JITCompiler::TrustedImm32(1), op2GPR, temp);
+    
+    JITCompiler::Jump safeDenominator = m_jit.branch32(JITCompiler::Above, temp, JITCompiler::TrustedImm32(1));
+    
+    JITCompiler::Jump done;
+    if (nodeUsedAsNumber(node.arithNodeFlags())) {
+        speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, op2GPR));
+        speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branch32(JITCompiler::Equal, op1GPR, TrustedImm32(-2147483648)));
+    } else {
+        JITCompiler::Jump zero = m_jit.branchTest32(JITCompiler::Zero, op2GPR);
+        JITCompiler::Jump notNeg2ToThe31 = m_jit.branch32(JITCompiler::Equal, op1GPR, TrustedImm32(-2147483648));
+        zero.link(&m_jit);
+        m_jit.move(TrustedImm32(0), eax.gpr());
+        done = m_jit.jump();
+        notNeg2ToThe31.link(&m_jit);
+    }
+    
+    safeDenominator.link(&m_jit);
+            
+    // If the user cares about negative zero, then speculate that we're not about
+    // to produce negative zero.
+    if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
+        MacroAssembler::Jump numeratorNonZero = m_jit.branchTest32(MacroAssembler::NonZero, op1GPR);
+        speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::LessThan, op2GPR, TrustedImm32(0)));
+        numeratorNonZero.link(&m_jit);
+    }
+    
+    if (op2TempGPR != InvalidGPRReg) {
+        m_jit.move(op2GPR, op2TempGPR);
+        op2GPR = op2TempGPR;
+    }
+            
+    m_jit.move(op1GPR, eax.gpr());
+    m_jit.assembler().cdq();
+    m_jit.assembler().idivl_r(op2GPR);
+            
+    if (op2TempGPR != InvalidGPRReg)
+        unlock(op2TempGPR);
+
+    // Check that there was no remainder. If there had been, then we'd be obligated to
+    // produce a double result instead.
+    if (nodeUsedAsNumber(node.arithNodeFlags()))
+        speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::NonZero, edx.gpr()));
+    else
+        done.link(&m_jit);
+            
+    integerResult(eax.gpr(), m_compileIndex);
+}
+#endif // CPU(X86) || CPU(X86_64)
+
 void SpeculativeJIT::compileArithMod(Node& node)
 {
     if (!at(node.child1()).shouldNotSpeculateInteger() && !at(node.child2()).shouldNotSpeculateInteger()

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h (111354 => 111355)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.h	2012-03-20 05:15:50 UTC (rev 111355)
@@ -1737,6 +1737,9 @@
     void compileArithSub(Node&);
     void compileArithNegate(Node&);
     void compileArithMul(Node&);
+#if CPU(X86) || CPU(X86_64)
+    void compileIntegerArithDivForX86(Node&);
+#endif
     void compileArithMod(Node&);
     void compileSoftModulo(Node&);
     void compileGetTypedArrayLength(const TypedArrayDescriptor&, Node&, bool needsSpeculationCheck);

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (111354 => 111355)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2012-03-20 05:15:50 UTC (rev 111355)
@@ -1899,43 +1899,7 @@
     case ArithDiv: {
         if (Node::shouldSpeculateInteger(at(node.child1()), at(node.child2())) && node.canSpeculateInteger()) {
 #if CPU(X86)
-            SpeculateIntegerOperand op1(this, node.child1());
-            SpeculateIntegerOperand op2(this, node.child2());
-            GPRReg op1GPR = op1.gpr();
-            GPRReg op2GPR = op2.gpr();
-            
-            speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, op2GPR));
-            
-            // If the user cares about negative zero, then speculate that we're not about
-            // to produce negative zero.
-            if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
-                MacroAssembler::Jump numeratorNonZero = m_jit.branchTest32(MacroAssembler::NonZero, op1GPR);
-                speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::LessThan, op2GPR, TrustedImm32(0)));
-                numeratorNonZero.link(&m_jit);
-            }
-            
-            GPRTemporary eax(this, X86Registers::eax);
-            GPRTemporary edx(this, X86Registers::edx);
-
-            GPRReg temp2 = InvalidGPRReg;
-            if (op2GPR == X86Registers::eax || op2GPR == X86Registers::edx) {
-                temp2 = allocate();
-                m_jit.move(op2GPR, temp2);
-                op2GPR = temp2;
-            }
-            
-            m_jit.move(op1GPR, eax.gpr());
-            m_jit.assembler().cdq();
-            m_jit.assembler().idivl_r(op2GPR);
-            
-            if (temp2 != InvalidGPRReg)
-                unlock(temp2);
-
-            // Check that there was no remainder. If there had been, then we'd be obligated to
-            // produce a double result instead.
-            speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::NonZero, edx.gpr()));
-            
-            integerResult(eax.gpr(), m_compileIndex);
+            compileIntegerArithDivForX86(node);
 #else // CPU(X86) -> so non-X86 code follows
             SpeculateDoubleOperand op1(this, node.child1());
             SpeculateDoubleOperand op2(this, node.child2());

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (111354 => 111355)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2012-03-20 05:15:50 UTC (rev 111355)
@@ -1984,42 +1984,7 @@
 
     case ArithDiv: {
         if (Node::shouldSpeculateInteger(at(node.child1()), at(node.child2())) && node.canSpeculateInteger()) {
-            SpeculateIntegerOperand op1(this, node.child1());
-            SpeculateIntegerOperand op2(this, node.child2());
-            GPRTemporary eax(this, X86Registers::eax);
-            GPRTemporary edx(this, X86Registers::edx);
-            GPRReg op1GPR = op1.gpr();
-            GPRReg op2GPR = op2.gpr();
-            
-            speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::Zero, op2GPR));
-            
-            // If the user cares about negative zero, then speculate that we're not about
-            // to produce negative zero.
-            if (!nodeCanIgnoreNegativeZero(node.arithNodeFlags())) {
-                MacroAssembler::Jump numeratorNonZero = m_jit.branchTest32(MacroAssembler::NonZero, op1GPR);
-                speculationCheck(NegativeZero, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::LessThan, op2GPR, TrustedImm32(0)));
-                numeratorNonZero.link(&m_jit);
-            }
-            
-            GPRReg temp2 = InvalidGPRReg;
-            if (op2GPR == X86Registers::eax || op2GPR == X86Registers::edx) {
-                temp2 = allocate();
-                m_jit.move(op2GPR, temp2);
-                op2GPR = temp2;
-            }
-            
-            m_jit.move(op1GPR, eax.gpr());
-            m_jit.assembler().cdq();
-            m_jit.assembler().idivl_r(op2GPR);
-            
-            if (temp2 != InvalidGPRReg)
-                unlock(temp2);
-
-            // Check that there was no remainder. If there had been, then we'd be obligated to
-            // produce a double result instead.
-            speculationCheck(Overflow, JSValueRegs(), NoNode, m_jit.branchTest32(JITCompiler::NonZero, edx.gpr()));
-            
-            integerResult(eax.gpr(), m_compileIndex);
+            compileIntegerArithDivForX86(node);
             break;
         }
         

Modified: trunk/Source/_javascript_Core/llint/LowLevelInterpreter64.asm (111354 => 111355)


--- trunk/Source/_javascript_Core/llint/LowLevelInterpreter64.asm	2012-03-20 05:04:58 UTC (rev 111354)
+++ trunk/Source/_javascript_Core/llint/LowLevelInterpreter64.asm	2012-03-20 05:15:50 UTC (rev 111355)
@@ -594,6 +594,9 @@
         macro (left, right, slow, index)
             # Assume t3 is scratchable.
             btiz left, slow
+            bineq left, -1, .notNeg2TwoThe31DivByNeg1
+            bieq right, -2147483648, .slow
+        .notNeg2TwoThe31DivByNeg1:
             btinz right, .intOK
             bilt left, 0, slow
         .intOK:
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to