Attachment 220550 Details for Bug 126600 – the patch

[patch] the patch

blah.patch (text/plain), 16.83 KB, created by Filip Pizlo on 2014-01-07 14:30:39 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2014-01-07 14:30:39 PST

Size: 16.83 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 161457)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2014-01-07  Filip Pizlo  <fpizlo@apple.com>
>+
>+        DFG fixup phase should be responsible for inserting ValueToInt32's as needed and it should use Phantom to keep the original values alive in case of OSR exit
>+        https://bugs.webkit.org/show_bug.cgi?id=126600
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        This fixes an embarrassing OSR exit liveness bug. It also simplifies the code. We were
>+        already using FixupPhase as the place where conversion nodes get inserted. ValueToInt32
>+        was the only exception to that rule, and that was one of the reasons why we had this bug.
>+
>+        * dfg/DFGBackwardsPropagationPhase.cpp:
>+        (JSC::DFG::BackwardsPropagationPhase::isWithinPowerOfTwo):
>+        (JSC::DFG::BackwardsPropagationPhase::propagate):
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::handleIntrinsic):
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+        * dfg/DFGFixupPhase.cpp:
>+        (JSC::DFG::FixupPhase::fixupNode):
>+        (JSC::DFG::FixupPhase::fixIntEdge):
>+        (JSC::DFG::FixupPhase::fixBinaryIntEdges):
>+        * dfg/DFGPredictionPropagationPhase.cpp:
>+        (JSC::DFG::PredictionPropagationPhase::propagate):
>+        * tests/stress/bit-op-value-to-int32-input-liveness.js: Added.
>+        (foo):
>+
> 2014-01-07  Mark Hahnenberg  <mhahnenberg@apple.com>
> 
>         Repatch write barrier slow path call doesn't align the stack in the presence of saved registers
>Index: Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp	(revision 161453)
>+++ Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp	(working copy)
>@@ -114,8 +114,7 @@ private:
>             
>         case BitOr:
>         case BitXor:
>-        case BitLShift:
>-        case ValueToInt32: {
>+        case BitLShift: {
>             return power > 31;
>         }
>             
>@@ -205,13 +204,6 @@ private:
>             break;
>         }
>             
>-        case ValueToInt32: {
>-            flags |= NodeBytecodeUsesAsInt;
>-            flags &= ~(NodeBytecodeUsesAsNumber | NodeBytecodeNeedsNegZero | NodeBytecodeUsesAsOther);
>-            node->child1()->mergeFlags(flags);
>-            break;
>-        }
>-            
>         case StringCharCodeAt: {
>             node->child1()->mergeFlags(NodeBytecodeUsesAsValue);
>             node->child2()->mergeFlags(NodeBytecodeUsesAsValue | NodeBytecodeUsesAsInt);
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 161453)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -505,30 +505,6 @@ private:
>         flush(m_inlineStackTop);
>     }
> 
>-    // Get an operand, and perform a ToInt32/ToNumber conversion on it.
>-    Node* getToInt32(int operand)
>-    {
>-        return toInt32(get(VirtualRegister(operand)));
>-    }
>-
>-    // Perform an ES5 ToInt32 operation - returns a node of type NodeResultInt32.
>-    Node* toInt32(Node* node)
>-    {
>-        if (node->hasInt32Result())
>-            return node;
>-
>-        // Check for numeric constants boxed as JSValues.
>-        if (canFold(node)) {
>-            JSValue v = valueOfJSConstant(node);
>-            if (v.isInt32())
>-                return getJSConstant(node->constantNumber());
>-            if (v.isNumber())
>-                return getJSConstantForValue(JSValue(JSC::toInt32(v.asNumber())));
>-        }
>-
>-        return addToGraph(ValueToInt32, node);
>-    }
>-
>     // NOTE: Only use this to construct constants that arise from non-speculative
>     // constant folding. I.e. creating constants using this if we had constant
>     // field inference would be a bad idea, since the bytecode parser's folding
>@@ -1577,9 +1553,9 @@ bool ByteCodeParser::handleIntrinsic(int
>         if (argumentCountIncludingThis != 2)
>             return false;
> 
>-        int thisOperand = virtualRegisterForArgument(0, registerOffset).offset();
>-        int indexOperand = virtualRegisterForArgument(1, registerOffset).offset();
>-        Node* charCode = addToGraph(StringCharCodeAt, OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)), getToInt32(indexOperand));
>+        VirtualRegister thisOperand = virtualRegisterForArgument(0, registerOffset);
>+        VirtualRegister indexOperand = virtualRegisterForArgument(1, registerOffset);
>+        Node* charCode = addToGraph(StringCharCodeAt, OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)), get(indexOperand));
> 
>         set(VirtualRegister(resultOperand), charCode);
>         return true;
>@@ -1589,9 +1565,9 @@ bool ByteCodeParser::handleIntrinsic(int
>         if (argumentCountIncludingThis != 2)
>             return false;
> 
>-        int thisOperand = virtualRegisterForArgument(0, registerOffset).offset();
>-        int indexOperand = virtualRegisterForArgument(1, registerOffset).offset();
>-        Node* charCode = addToGraph(StringCharAt, OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)), getToInt32(indexOperand));
>+        VirtualRegister thisOperand = virtualRegisterForArgument(0, registerOffset);
>+        VirtualRegister indexOperand = virtualRegisterForArgument(1, registerOffset);
>+        Node* charCode = addToGraph(StringCharAt, OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)), get(indexOperand));
> 
>         set(VirtualRegister(resultOperand), charCode);
>         return true;
>@@ -1600,8 +1576,8 @@ bool ByteCodeParser::handleIntrinsic(int
>         if (argumentCountIncludingThis != 2)
>             return false;
> 
>-        int indexOperand = virtualRegisterForArgument(1, registerOffset).offset();
>-        Node* charCode = addToGraph(StringFromCharCode, getToInt32(indexOperand));
>+        VirtualRegister indexOperand = virtualRegisterForArgument(1, registerOffset);
>+        Node* charCode = addToGraph(StringFromCharCode, get(indexOperand));
> 
>         set(VirtualRegister(resultOperand), charCode);
> 
>@@ -1631,10 +1607,10 @@ bool ByteCodeParser::handleIntrinsic(int
>     case IMulIntrinsic: {
>         if (argumentCountIncludingThis != 3)
>             return false;
>-        int leftOperand = virtualRegisterForArgument(1, registerOffset).offset();
>-        int rightOperand = virtualRegisterForArgument(2, registerOffset).offset();
>-        Node* left = getToInt32(leftOperand);
>-        Node* right = getToInt32(rightOperand);
>+        VirtualRegister leftOperand = virtualRegisterForArgument(1, registerOffset);
>+        VirtualRegister rightOperand = virtualRegisterForArgument(2, registerOffset);
>+        Node* left = get(leftOperand);
>+        Node* right = get(rightOperand);
>         set(VirtualRegister(resultOperand), addToGraph(ArithIMul, left, right));
>         return true;
>     }
>@@ -2053,45 +2029,45 @@ bool ByteCodeParser::parseBlock(unsigned
>         // === Bitwise operations ===
> 
>         case op_bitand: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(BitAnd, op1, op2));
>             NEXT_OPCODE(op_bitand);
>         }
> 
>         case op_bitor: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(BitOr, op1, op2));
>             NEXT_OPCODE(op_bitor);
>         }
> 
>         case op_bitxor: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(BitXor, op1, op2));
>             NEXT_OPCODE(op_bitxor);
>         }
> 
>         case op_rshift: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand),
>                 addToGraph(BitRShift, op1, op2));
>             NEXT_OPCODE(op_rshift);
>         }
> 
>         case op_lshift: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand),
>                 addToGraph(BitLShift, op1, op2));
>             NEXT_OPCODE(op_lshift);
>         }
> 
>         case op_urshift: {
>-            Node* op1 = getToInt32(currentInstruction[2].u.operand);
>-            Node* op2 = getToInt32(currentInstruction[3].u.operand);
>+            Node* op1 = get(VirtualRegister(currentInstruction[2].u.operand));
>+            Node* op2 = get(VirtualRegister(currentInstruction[3].u.operand));
>             set(VirtualRegister(currentInstruction[1].u.operand),
>                 addToGraph(BitURShift, op1, op2));
>             NEXT_OPCODE(op_urshift);
>@@ -2099,7 +2075,7 @@ bool ByteCodeParser::parseBlock(unsigned
>             
>         case op_unsigned: {
>             set(VirtualRegister(currentInstruction[1].u.operand),
>-                makeSafe(addToGraph(UInt32ToNumber, getToInt32(currentInstruction[2].u.operand))));
>+                makeSafe(addToGraph(UInt32ToNumber, get(VirtualRegister(currentInstruction[2].u.operand)))));
>             NEXT_OPCODE(op_unsigned);
>         }
> 
>Index: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(revision 161453)
>+++ Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(working copy)
>@@ -94,24 +94,18 @@ private:
>             return;
>         }
>             
>-        case BitOr: {
>-            fixIntEdge(node->child1());
>-            fixIntEdge(node->child2());
>-            break;
>-        }
>         case BitAnd:
>+        case BitOr:
>         case BitXor:
>         case BitRShift:
>         case BitLShift:
>         case BitURShift: {
>-            fixIntEdge(node->child1());
>-            fixIntEdge(node->child2());
>+            fixBinaryIntEdges();
>             break;
>         }
>-            
>+
>         case ArithIMul: {
>-            fixIntEdge(node->child1());
>-            fixIntEdge(node->child2());
>+            fixBinaryIntEdges();
>             node->setOp(ArithMul);
>             node->setArithMode(Arith::Unchecked);
>             node->child1().setUseKind(Int32Use);
>@@ -120,7 +114,7 @@ private:
>         }
>             
>         case UInt32ToNumber: {
>-            fixEdge<KnownInt32Use>(node->child1());
>+            fixIntEdge(node->child1());
>             if (bytecodeCanTruncateInteger(node->arithNodeFlags()))
>                 node->convertToIdentity();
>             else if (nodeCanSpeculateInt32(node->arithNodeFlags()))
>@@ -130,42 +124,6 @@ private:
>             break;
>         }
>             
>-        case DoubleAsInt32: {
>-            RELEASE_ASSERT_NOT_REACHED();
>-            break;
>-        }
>-            
>-        case ValueToInt32: {
>-            if (node->child1()->shouldSpeculateInt32()) {
>-                fixEdge<Int32Use>(node->child1());
>-                node->setOpAndDefaultFlags(Identity);
>-                break;
>-            }
>-            
>-            if (node->child1()->shouldSpeculateMachineInt()) {
>-                fixEdge<MachineIntUse>(node->child1());
>-                break;
>-            }
>-            
>-            if (node->child1()->shouldSpeculateNumber()) {
>-                fixEdge<NumberUse>(node->child1());
>-                break;
>-            }
>-            
>-            if (node->child1()->shouldSpeculateBoolean()) {
>-                fixEdge<BooleanUse>(node->child1());
>-                break;
>-            }
>-            
>-            fixEdge<NotCellUse>(node->child1());
>-            break;
>-        }
>-            
>-        case Int32ToDouble: {
>-            RELEASE_ASSERT_NOT_REACHED();
>-            break;
>-        }
>-            
>         case ValueAdd: {
>             if (attemptToMakeIntegerAdd(node)) {
>                 node->setOp(ArithAdd);
>@@ -974,6 +932,9 @@ private:
>         case CheckArray:
>         case CheckInBounds:
>         case ConstantStoragePointer:
>+        case DoubleAsInt32:
>+        case Int32ToDouble:
>+        case ValueToInt32:
>             // These are just nodes that we don't currently expect to see during fixup.
>             // If we ever wanted to insert them prior to fixup, then we just have to create
>             // fixup rules for them.
>@@ -1624,23 +1585,40 @@ private:
>         m_insertionSet.insert(indexInBlock, barrierNode);
>     }
> 
>-    void fixIntEdge(Edge& edge)
>+    bool fixIntEdge(Edge& edge)
>     {
>         Node* node = edge.node();
>-        if (node->op() != ValueToInt32) {
>-            fixEdge<KnownInt32Use>(edge);
>-            return;
>+        if (node->shouldSpeculateInt32()) {
>+            fixEdge<Int32Use>(edge);
>+            return false;
>         }
>         
>-        Edge newEdge = node->child1();
>+        UseKind useKind;
>+        if (node->shouldSpeculateMachineInt())
>+            useKind = MachineIntUse;
>+        else if (node->shouldSpeculateNumber())
>+            useKind = NumberUse;
>+        else if (node->shouldSpeculateBoolean())
>+            useKind = BooleanUse;
>+        else
>+            useKind = NotCellUse;
>+        Node* newNode = m_insertionSet.insertNode(
>+            m_indexInBlock, SpecInt32, ValueToInt32, m_currentNode->codeOrigin,
>+            Edge(node, useKind));
>+        observeUseKindOnNode(node, useKind);
>         
>-        if (newEdge.useKind() != Int32Use) {
>-            edge.setUseKind(KnownInt32Use);
>+        edge = Edge(newNode, KnownInt32Use);
>+        return true;
>+    }
>+    
>+    void fixBinaryIntEdges()
>+    {
>+        AdjacencyList children = m_currentNode->children;
>+        bool needPhantom =
>+            fixIntEdge(m_currentNode->child1()) | fixIntEdge(m_currentNode->child2());
>+        if (!needPhantom)
>             return;
>-        }
>-        
>-        ASSERT(newEdge->shouldSpeculateInt32());
>-        edge = newEdge;
>+        m_insertionSet.insertNode(m_indexInBlock + 1, SpecNone, Phantom, m_currentNode->codeOrigin, children);
>     }
>     
>     void injectInt32ToDoubleNode(Edge& edge, UseKind useKind = NumberUse)
>Index: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(revision 161453)
>+++ Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(working copy)
>@@ -168,11 +168,6 @@ private:
>             break;
>         }
>             
>-        case ValueToInt32: {
>-            changed |= setPrediction(SpecInt32);
>-            break;
>-        }
>-            
>         case ArrayPop:
>         case ArrayPush:
>         case RegExpExec:
>@@ -510,7 +505,8 @@ private:
>         case InvalidationPoint:
>         case Int52ToValue:
>         case Int52ToDouble:
>-        case CheckInBounds: {
>+        case CheckInBounds:
>+        case ValueToInt32: {
>             // This node should never be visible at this stage of compilation. It is
>             // inserted by fixup(), which follows this phase.
>             RELEASE_ASSERT_NOT_REACHED();
>Index: Source/JavaScriptCore/tests/stress/bit-op-value-to-int32-input-liveness.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/bit-op-value-to-int32-input-liveness.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/bit-op-value-to-int32-input-liveness.js	(working copy)
>@@ -0,0 +1,20 @@
>+function foo(a, b) {
>+    return a.f ^ b.f;
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 100000; ++i) {
>+    var result = foo({f:5.5}, {f:6.5});
>+    if (result != 3)
>+        throw "Error: bad result: " + result;
>+}
>+
>+var result = foo({f:"5.5"}, {f:6.5});
>+if (result != 3)
>+    throw "Error: bad result: " + result;
>+
>+var result = foo({f:5.5}, {f:"6.5"});
>+if (result != 3)
>+    throw "Error: bad result: " + result;
>+

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 126600: 220550