Log Message
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 Michael Saboff.
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.
Henceforth ValueToInt32 is only inserted by FixupPhase, and only when it is necessary:
we have a BitOp that will want a ToInt32 conversion and the operand is not predicted to
already be an int32. If FixupPhase inserts any ValueToInt32's then the BitOp will no
longer appear to use the original operand, which will make OSR exit think that the
original operand is dead. We work around this they way we always do: insert a Phantom on
the original operands right after the BitOp. This ensures that any OSR exit in any of the
ValueToInt32's or in the BitOp itself will have values for the original inputs.
* 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):
Modified Paths
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp
- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp
- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp
- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (161464 => 161465)
--- trunk/Source/_javascript_Core/ChangeLog 2014-01-08 00:12:03 UTC (rev 161464)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-01-08 00:27:06 UTC (rev 161465)
@@ -1,3 +1,37 @@
+2014-01-07 Filip Pizlo <fpi...@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 Michael Saboff.
+
+ 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.
+
+ Henceforth ValueToInt32 is only inserted by FixupPhase, and only when it is necessary:
+ we have a BitOp that will want a ToInt32 conversion and the operand is not predicted to
+ already be an int32. If FixupPhase inserts any ValueToInt32's then the BitOp will no
+ longer appear to use the original operand, which will make OSR exit think that the
+ original operand is dead. We work around this they way we always do: insert a Phantom on
+ the original operands right after the BitOp. This ensures that any OSR exit in any of the
+ ValueToInt32's or in the BitOp itself will have values for the original inputs.
+
+ * 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 <mhahnenb...@apple.com>
Repatch write barrier slow path call doesn't align the stack in the presence of saved registers
Modified: trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp (161464 => 161465)
--- trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp 2014-01-08 00:12:03 UTC (rev 161464)
+++ trunk/Source/_javascript_Core/dfg/DFGBackwardsPropagationPhase.cpp 2014-01-08 00:27:06 UTC (rev 161465)
@@ -114,8 +114,7 @@
case BitOr:
case BitXor:
- case BitLShift:
- case ValueToInt32: {
+ case BitLShift: {
return power > 31;
}
@@ -205,13 +204,6 @@
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);
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (161464 => 161465)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2014-01-08 00:12:03 UTC (rev 161464)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2014-01-08 00:27:06 UTC (rev 161465)
@@ -505,30 +505,6 @@
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 @@
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(thisOperand), get(indexOperand));
set(VirtualRegister(resultOperand), charCode);
return true;
@@ -1589,9 +1565,9 @@
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(thisOperand), get(indexOperand));
set(VirtualRegister(resultOperand), charCode);
return true;
@@ -1600,8 +1576,8 @@
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 @@
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 @@
// === 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 @@
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);
}
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (161464 => 161465)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2014-01-08 00:12:03 UTC (rev 161464)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2014-01-08 00:27:06 UTC (rev 161465)
@@ -94,24 +94,18 @@
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 @@
}
case UInt32ToNumber: {
- fixEdge<KnownInt32Use>(node->child1());
+ fixIntEdge(node->child1());
if (bytecodeCanTruncateInteger(node->arithNodeFlags()))
node->convertToIdentity();
else if (nodeCanSpeculateInt32(node->arithNodeFlags()))
@@ -130,42 +124,6 @@
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 @@
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,43 @@
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);
- return;
- }
+ edge = Edge(newNode, KnownInt32Use);
+ return true;
+ }
+
+ void fixBinaryIntEdges()
+ {
+ AdjacencyList children = m_currentNode->children;
- ASSERT(newEdge->shouldSpeculateInt32());
- edge = newEdge;
+ // Call fixIntEdge() on both edges.
+ bool needPhantom =
+ fixIntEdge(m_currentNode->child1()) | fixIntEdge(m_currentNode->child2());
+
+ if (!needPhantom)
+ return;
+ m_insertionSet.insertNode(m_indexInBlock + 1, SpecNone, Phantom, m_currentNode->codeOrigin, children);
}
void injectInt32ToDoubleNode(Edge& edge, UseKind useKind = NumberUse)
Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (161464 => 161465)
--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2014-01-08 00:12:03 UTC (rev 161464)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2014-01-08 00:27:06 UTC (rev 161465)
@@ -168,11 +168,6 @@
break;
}
- case ValueToInt32: {
- changed |= setPrediction(SpecInt32);
- break;
- }
-
case ArrayPop:
case ArrayPush:
case RegExpExec:
@@ -510,7 +505,8 @@
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();
Added: trunk/Source/_javascript_Core/tests/stress/bit-op-value-to-int32-input-liveness.js (0 => 161465)
--- trunk/Source/_javascript_Core/tests/stress/bit-op-value-to-int32-input-liveness.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/bit-op-value-to-int32-input-liveness.js 2014-01-08 00:27:06 UTC (rev 161465)
@@ -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;
+
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
