Attachment 222357 Details for Bug 127709 – the patch

[patch] the patch

blah.patch (text/plain), 10.14 KB, created by Filip Pizlo on 2014-01-27 14:25:51 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2014-01-27 14:25:51 PST

Size: 10.14 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 162867)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2014-01-27  Filip Pizlo  <fpizlo@apple.com>
>+
>+        FTL ArithMod case needs to be factored for hackability and correctness rather than just minimizing the amount of code
>+        https://bugs.webkit.org/show_bug.cgi?id=127709
>+
>+        Rubber stamped by Mark Hahnenberg.
>+        
>+        Roll out part of r156784.
>+
>+        * ftl/FTLLowerDFGToLLVM.cpp:
>+        (JSC::FTL::LowerDFGToLLVM::compileNode):
>+        (JSC::FTL::LowerDFGToLLVM::compileArithDiv):
>+        (JSC::FTL::LowerDFGToLLVM::compileArithMod):
>+
> 2014-01-27  Mark Hahnenberg  <mhahnenberg@apple.com>
> 
>         Added missing files to the branch.
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(revision 162867)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(working copy)
>@@ -314,8 +314,10 @@ private:
>             compileArithMul();
>             break;
>         case ArithDiv:
>+            compileArithDiv();
>+            break;
>         case ArithMod:
>-            compileArithDivMod();
>+            compileArithMod();
>             break;
>         case ArithMin:
>         case ArithMax:
>@@ -1074,16 +1076,16 @@ private:
>         }
>     }
> 
>-    void compileArithDivMod()
>+    void compileArithDiv()
>     {
>         switch (m_node->binaryUseKind()) {
>         case Int32Use: {
>             LValue numerator = lowInt32(m_node->child1());
>             LValue denominator = lowInt32(m_node->child2());
>             
>-            LBasicBlock unsafeDenominator = FTL_NEW_BLOCK(m_out, ("ArithDivMod unsafe denominator"));
>-            LBasicBlock continuation = FTL_NEW_BLOCK(m_out, ("ArithDivMod continuation"));
>-            LBasicBlock done = FTL_NEW_BLOCK(m_out, ("ArithDivMod done"));
>+            LBasicBlock unsafeDenominator = FTL_NEW_BLOCK(m_out, ("ArithDiv unsafe denominator"));
>+            LBasicBlock continuation = FTL_NEW_BLOCK(m_out, ("ArithDiv continuation"));
>+            LBasicBlock done = FTL_NEW_BLOCK(m_out, ("ArithDiv done"));
>             
>             Vector<ValueFromBlock, 3> results;
>             
>@@ -1105,9 +1107,9 @@ private:
>                 // If the denominator is not zero (i.e. it's -1 because we're guarded by the
>                 // check above) and the numerator is -2^31 then the result should be -2^31.
>                 
>-                LBasicBlock divByZero = FTL_NEW_BLOCK(m_out, ("ArithDivMod divide by zero"));
>-                LBasicBlock notDivByZero = FTL_NEW_BLOCK(m_out, ("ArithDivMod not divide by zero"));
>-                LBasicBlock neg2ToThe31ByNeg1 = FTL_NEW_BLOCK(m_out, ("ArithDivMod -2^31/-1"));
>+                LBasicBlock divByZero = FTL_NEW_BLOCK(m_out, ("ArithDiv divide by zero"));
>+                LBasicBlock notDivByZero = FTL_NEW_BLOCK(m_out, ("ArithDiv not divide by zero"));
>+                LBasicBlock neg2ToThe31ByNeg1 = FTL_NEW_BLOCK(m_out, ("ArithDiv -2^31/-1"));
>                 
>                 m_out.branch(m_out.isZero32(denominator), divByZero, notDivByZero);
>                 
>@@ -1119,18 +1121,15 @@ private:
>                 m_out.branch(m_out.equal(numerator, neg2ToThe31), neg2ToThe31ByNeg1, continuation);
>                 
>                 m_out.appendTo(neg2ToThe31ByNeg1, continuation);
>-                if (m_node->op() == ArithDiv)
>-                    results.append(m_out.anchor(neg2ToThe31));
>-                else
>-                    results.append(m_out.anchor(m_out.int32Zero));
>+                results.append(m_out.anchor(neg2ToThe31));
>                 m_out.jump(done);
>             }
>             
>             m_out.appendTo(continuation, done);
>             
>             if (shouldCheckNegativeZero(m_node->arithMode())) {
>-                LBasicBlock zeroNumerator = FTL_NEW_BLOCK(m_out, ("ArithDivMod zero numerator"));
>-                LBasicBlock numeratorContinuation = FTL_NEW_BLOCK(m_out, ("ArithDivMod numerator continuation"));
>+                LBasicBlock zeroNumerator = FTL_NEW_BLOCK(m_out, ("ArithDiv zero numerator"));
>+                LBasicBlock numeratorContinuation = FTL_NEW_BLOCK(m_out, ("ArithDiv numerator continuation"));
>                 
>                 m_out.branch(m_out.isZero32(numerator), zeroNumerator, numeratorContinuation);
>                 
>@@ -1144,17 +1143,15 @@ private:
>                 m_out.appendTo(numeratorContinuation, innerLastNext);
>             }
>             
>-            LValue divModResult = m_node->op() == ArithDiv
>-                ? m_out.div(numerator, denominator)
>-                : m_out.rem(numerator, denominator);
>+            LValue result = m_out.div(numerator, denominator);
>             
>             if (shouldCheckOverflow(m_node->arithMode())) {
>                 speculate(
>                     Overflow, noValue(), 0,
>-                    m_out.notEqual(m_out.mul(divModResult, denominator), numerator));
>+                    m_out.notEqual(m_out.mul(result, denominator), numerator));
>             }
>             
>-            results.append(m_out.anchor(divModResult));
>+            results.append(m_out.anchor(result));
>             m_out.jump(done);
>             
>             m_out.appendTo(done, lastNext);
>@@ -1164,9 +1161,8 @@ private:
>         }
>             
>         case NumberUse: {
>-            LValue C1 = lowDouble(m_node->child1());
>-            LValue C2 = lowDouble(m_node->child2());
>-            setDouble(m_node->op() == ArithDiv ? m_out.doubleDiv(C1, C2) : m_out.doubleRem(C1, C2));
>+            setDouble(m_out.doubleDiv(
>+                lowDouble(m_node->child1()), lowDouble(m_node->child2())));
>             break;
>         }
>             
>@@ -1176,6 +1172,99 @@ private:
>         }
>     }
>     
>+    void compileArithMod()
>+    {
>+        switch (m_node->binaryUseKind()) {
>+        case Int32Use: {
>+            LValue numerator = lowInt32(m_node->child1());
>+            LValue denominator = lowInt32(m_node->child2());
>+            
>+            LBasicBlock unsafeDenominator = FTL_NEW_BLOCK(m_out, ("ArithMod unsafe denominator"));
>+            LBasicBlock continuation = FTL_NEW_BLOCK(m_out, ("ArithMod continuation"));
>+            LBasicBlock done = FTL_NEW_BLOCK(m_out, ("ArithMod done"));
>+            
>+            Vector<ValueFromBlock, 3> results;
>+            
>+            LValue adjustedDenominator = m_out.add(denominator, m_out.int32One);
>+            
>+            m_out.branch(m_out.above(adjustedDenominator, m_out.int32One), continuation, unsafeDenominator);
>+            
>+            LBasicBlock lastNext = m_out.appendTo(unsafeDenominator, continuation);
>+            
>+            LValue neg2ToThe31 = m_out.constInt32(-2147483647-1);
>+            
>+            // FIXME: -2^31 / -1 will actually yield negative zero, so we could have a
>+            // separate case for that. But it probably doesn't matter so much.
>+            if (shouldCheckOverflow(m_node->arithMode())) {
>+                LValue cond = m_out.bitOr(m_out.isZero32(denominator), m_out.equal(numerator, neg2ToThe31));
>+                speculate(Overflow, noValue(), 0, cond);
>+                m_out.jump(continuation);
>+            } else {
>+                // This is the case where we convert the result to an int after we're done. So,
>+                // if the denominator is zero, then the result should be result should be zero.
>+                // If the denominator is not zero (i.e. it's -1 because we're guarded by the
>+                // check above) and the numerator is -2^31 then the result should be -2^31.
>+                
>+                LBasicBlock modByZero = FTL_NEW_BLOCK(m_out, ("ArithMod modulo by zero"));
>+                LBasicBlock notModByZero = FTL_NEW_BLOCK(m_out, ("ArithMod not modulo by zero"));
>+                LBasicBlock neg2ToThe31ByNeg1 = FTL_NEW_BLOCK(m_out, ("ArithMod -2^31/-1"));
>+                
>+                m_out.branch(m_out.isZero32(denominator), modByZero, notModByZero);
>+                
>+                m_out.appendTo(modByZero, notModByZero);
>+                results.append(m_out.anchor(m_out.int32Zero));
>+                m_out.jump(done);
>+                
>+                m_out.appendTo(notModByZero, neg2ToThe31ByNeg1);
>+                m_out.branch(m_out.equal(numerator, neg2ToThe31), neg2ToThe31ByNeg1, continuation);
>+                
>+                m_out.appendTo(neg2ToThe31ByNeg1, continuation);
>+                results.append(m_out.anchor(m_out.int32Zero));
>+                m_out.jump(done);
>+            }
>+            
>+            m_out.appendTo(continuation, done);
>+            
>+            LValue remainder = m_out.rem(numerator, denominator);
>+            
>+            if (shouldCheckNegativeZero(m_node->arithMode())) {
>+                LBasicBlock negativeNumerator = FTL_NEW_BLOCK(m_out, ("ArithMod negative numerator"));
>+                LBasicBlock numeratorContinuation = FTL_NEW_BLOCK(m_out, ("ArithMod numerator continuation"));
>+                
>+                m_out.branch(
>+                    m_out.lessThan(numerator, m_out.int32Zero),
>+                    negativeNumerator, numeratorContinuation);
>+                
>+                LBasicBlock innerLastNext = m_out.appendTo(negativeNumerator, numeratorContinuation);
>+                
>+                speculate(NegativeZero, noValue(), 0, m_out.isZero32(remainder));
>+                
>+                m_out.jump(numeratorContinuation);
>+                
>+                m_out.appendTo(numeratorContinuation, innerLastNext);
>+            }
>+            
>+            results.append(m_out.anchor(remainder));
>+            m_out.jump(done);
>+            
>+            m_out.appendTo(done, lastNext);
>+            
>+            setInt32(m_out.phi(m_out.int32, results));
>+            break;
>+        }
>+            
>+        case NumberUse: {
>+            setDouble(
>+                m_out.doubleRem(lowDouble(m_node->child1()), lowDouble(m_node->child2())));
>+            break;
>+        }
>+            
>+        default:
>+            RELEASE_ASSERT_NOT_REACHED();
>+            break;
>+        }
>+    }
>+
>     void compileArithMinOrMax()
>     {
>         switch (m_node->binaryUseKind()) {

Actions: View | Formatted Diff | Diff

Attachments on bug 127709: 222357