Strange bug in DFG OSR in JSC
https://bugs.webkit.org/show_bug.cgi?id=109491
Source/JavaScriptCore:
Reviewed by Mark Hahnenberg.
Int32ToDouble was being injected after a side-effecting operation and before a SetLocal. Anytime we
inject something just before a SetLocal we should be aware that the previous operation may have been
a side-effect associated with the current code origin. Hence, we should use a forward exit.
Int32ToDouble does not do forward exits by default.
This patch adds a forward-exiting form of Int32ToDouble, for use in SetLocal Int32ToDouble injections.
Changed the CSE and other things to treat these nodes identically, but for the exit strategy to be
distinct (Int32ToDouble -> backward, ForwardInt32ToDouble -> forward). The use of the NodeType for
signaling exit direction is not "great" but it's what we use in other places already (like
ForwardCheckStructure).
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::execute):
* dfg/DFGCSEPhase.cpp:
(JSC::DFG::CSEPhase::int32ToDoubleCSE):
(CSEPhase):
(JSC::DFG::CSEPhase::performNodeCSE):
* dfg/DFGCommon.h:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::fixDoubleEdge):
(JSC::DFG::FixupPhase::injectInt32ToDoubleNode):
* dfg/DFGNode.h:
(JSC::DFG::Node::willHaveCodeGenOrOSR):
* dfg/DFGNodeType.h:
(DFG):
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::convertLastOSRExitToForward):
(JSC::DFG::SpeculativeJIT::compileInt32ToDouble):
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGVariableEventStream.cpp:
(JSC::DFG::VariableEventStream::reconstruct):
LayoutTests:
Reviewed by Mark Hahnenberg.
Added one version of the test (dfg-int32-to-double-on-set-local-and-exit) that is based
exactly on Gabor's original test, and another that ought to fail even if I fix other bugs
in the future (see https://bugs.webkit.org/show_bug.cgi?id=109511).
* fast/js/dfg-int32-to-double-on-set-local-and-exit-expected.txt: Added.
* fast/js/dfg-int32-to-double-on-set-local-and-exit.html: Added.
* fast/js/dfg-int32-to-double-on-set-local-and-sometimes-exit-expected.txt: Added.
* fast/js/dfg-int32-to-double-on-set-local-and-sometimes-exit.html: Added.
* fast/js/script-tests/dfg-int32-to-double-on-set-local-and-exit.js: Added.
(checkpoint):
(func1):
(func2):
(func3):
(test):
* fast/js/script-tests/dfg-int32-to-double-on-set-local-and-sometimes-exit.js: Added.
(checkpoint):
(func1):
(func2):
(func3):
(test):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@142544 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index b5e0412..99a3a78 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,5 +1,51 @@
2013-02-11 Filip Pizlo <fpizlo@apple.com>
+ Strange bug in DFG OSR in JSC
+ https://bugs.webkit.org/show_bug.cgi?id=109491
+
+ Reviewed by Mark Hahnenberg.
+
+ Int32ToDouble was being injected after a side-effecting operation and before a SetLocal. Anytime we
+ inject something just before a SetLocal we should be aware that the previous operation may have been
+ a side-effect associated with the current code origin. Hence, we should use a forward exit.
+ Int32ToDouble does not do forward exits by default.
+
+ This patch adds a forward-exiting form of Int32ToDouble, for use in SetLocal Int32ToDouble injections.
+ Changed the CSE and other things to treat these nodes identically, but for the exit strategy to be
+ distinct (Int32ToDouble -> backward, ForwardInt32ToDouble -> forward). The use of the NodeType for
+ signaling exit direction is not "great" but it's what we use in other places already (like
+ ForwardCheckStructure).
+
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::execute):
+ * dfg/DFGCSEPhase.cpp:
+ (JSC::DFG::CSEPhase::int32ToDoubleCSE):
+ (CSEPhase):
+ (JSC::DFG::CSEPhase::performNodeCSE):
+ * dfg/DFGCommon.h:
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ (JSC::DFG::FixupPhase::fixDoubleEdge):
+ (JSC::DFG::FixupPhase::injectInt32ToDoubleNode):
+ * dfg/DFGNode.h:
+ (JSC::DFG::Node::willHaveCodeGenOrOSR):
+ * dfg/DFGNodeType.h:
+ (DFG):
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ (JSC::DFG::PredictionPropagationPhase::propagate):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::convertLastOSRExitToForward):
+ (JSC::DFG::SpeculativeJIT::compileInt32ToDouble):
+ * dfg/DFGSpeculativeJIT.h:
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGVariableEventStream.cpp:
+ (JSC::DFG::VariableEventStream::reconstruct):
+
+2013-02-11 Filip Pizlo <fpizlo@apple.com>
+
NonStringCell and Object are practically the same thing for the purpose of speculation
https://bugs.webkit.org/show_bug.cgi?id=109492
diff --git a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
index cd14e28..17ad2e2 100644
--- a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
+++ b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
@@ -436,7 +436,8 @@
break;
}
- case Int32ToDouble: {
+ case Int32ToDouble:
+ case ForwardInt32ToDouble: {
JSValue child = forNode(node->child1()).value();
if (child && child.isNumber()
&& trySetConstant(node, JSValue(JSValue::EncodeAsDouble, child.asNumber()))) {
diff --git a/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp b/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
index 9745ce0..18495b5 100644
--- a/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
@@ -126,6 +126,27 @@
return 0;
}
+ Node* int32ToDoubleCSE(Node* node)
+ {
+ for (unsigned i = m_indexInBlock; i--;) {
+ Node* otherNode = m_currentBlock->at(i);
+ if (otherNode == node->child1())
+ return 0;
+ if (!otherNode->shouldGenerate())
+ continue;
+ switch (otherNode->op()) {
+ case Int32ToDouble:
+ case ForwardInt32ToDouble:
+ if (otherNode->child1() == node->child1())
+ return otherNode;
+ break;
+ default:
+ break;
+ }
+ }
+ return 0;
+ }
+
Node* constantCSE(Node* node)
{
for (unsigned i = endIndexForPureCSE(); i--;) {
@@ -1097,7 +1118,6 @@
case ArithSqrt:
case StringCharAt:
case StringCharCodeAt:
- case Int32ToDouble:
case IsUndefined:
case IsBoolean:
case IsNumber:
@@ -1115,6 +1135,11 @@
setReplacement(pureCSE(node));
break;
+ case Int32ToDouble:
+ case ForwardInt32ToDouble:
+ setReplacement(int32ToDoubleCSE(node));
+ break;
+
case GetCallee:
setReplacement(getCalleeLoadElimination(node->codeOrigin.inlineCallFrame));
break;
diff --git a/Source/JavaScriptCore/dfg/DFGCommon.h b/Source/JavaScriptCore/dfg/DFGCommon.h
index 7908b10..6a275cc 100644
--- a/Source/JavaScriptCore/dfg/DFGCommon.h
+++ b/Source/JavaScriptCore/dfg/DFGCommon.h
@@ -236,6 +236,8 @@
GloballyUnified
};
+enum SpeculationDirection { ForwardSpeculation, BackwardSpeculation };
+
} } // namespace JSC::DFG
namespace WTF {
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index 0217665..1a93765 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -252,7 +252,7 @@
break;
if (!node->variableAccessData()->shouldUseDoubleFormat())
break;
- fixDoubleEdge(0);
+ fixDoubleEdge(0, ForwardSpeculation);
break;
}
@@ -508,7 +508,7 @@
edge = newEdge;
}
- void fixDoubleEdge(unsigned childIndex)
+ void fixDoubleEdge(unsigned childIndex, SpeculationDirection direction = BackwardSpeculation)
{
Node* source = m_currentNode;
Edge& edge = m_graph.child(source, childIndex);
@@ -518,13 +518,14 @@
return;
}
- injectInt32ToDoubleNode(childIndex);
+ injectInt32ToDoubleNode(childIndex, direction);
}
- void injectInt32ToDoubleNode(unsigned childIndex)
+ void injectInt32ToDoubleNode(unsigned childIndex, SpeculationDirection direction = BackwardSpeculation)
{
Node* result = m_insertionSet.insertNode(
- m_indexInBlock, DontRefChildren, RefNode, SpecDouble, Int32ToDouble,
+ m_indexInBlock, DontRefChildren, RefNode, SpecDouble,
+ direction == BackwardSpeculation ? Int32ToDouble : ForwardInt32ToDouble,
m_currentNode->codeOrigin, m_graph.child(m_currentNode, childIndex).node());
#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
diff --git a/Source/JavaScriptCore/dfg/DFGNode.h b/Source/JavaScriptCore/dfg/DFGNode.h
index b818e8a..28ec073 100644
--- a/Source/JavaScriptCore/dfg/DFGNode.h
+++ b/Source/JavaScriptCore/dfg/DFGNode.h
@@ -943,6 +943,7 @@
switch (op()) {
case SetLocal:
case Int32ToDouble:
+ case ForwardInt32ToDouble:
case ValueToInt32:
case UInt32ToNumber:
case DoubleAsInt32:
diff --git a/Source/JavaScriptCore/dfg/DFGNodeType.h b/Source/JavaScriptCore/dfg/DFGNodeType.h
index 726b6f9..d4ae55f 100644
--- a/Source/JavaScriptCore/dfg/DFGNodeType.h
+++ b/Source/JavaScriptCore/dfg/DFGNodeType.h
@@ -93,6 +93,7 @@
/* Used to cast known integers to doubles, so as to separate the double form */\
/* of the value from the integer form. */\
macro(Int32ToDouble, NodeResultNumber) \
+ macro(ForwardInt32ToDouble, NodeResultNumber) \
/* Used to speculate that a double value is actually an integer. */\
macro(DoubleAsInt32, NodeResultInt32) \
/* Used to record places where we must check if a value is a number. */\
diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index 2b203a2..952fc76 100644
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -727,6 +727,7 @@
case PutByValAlias:
case GetArrayLength:
case Int32ToDouble:
+ case ForwardInt32ToDouble:
case DoubleAsInt32:
case GetLocalUnlinked:
case GetMyArgumentsLength:
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
index e0caf75..48247eb 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
@@ -234,7 +234,7 @@
Node* setLocal = m_jit.graph().m_blocks[m_block]->at(setLocalIndexInBlock);
bool hadInt32ToDouble = false;
- if (setLocal->op() == Int32ToDouble) {
+ if (setLocal->op() == ForwardInt32ToDouble) {
setLocal = m_jit.graph().m_blocks[m_block]->at(++setLocalIndexInBlock);
hadInt32ToDouble = true;
}
@@ -2395,9 +2395,16 @@
MacroAssembler::AboveOrEqual, op1GPR, GPRInfo::tagTypeNumberRegister);
if (!isNumberSpeculation(m_state.forNode(node->child1()).m_type)) {
- speculationCheck(
- BadType, JSValueRegs(op1GPR), node->child1(),
- m_jit.branchTest64(MacroAssembler::Zero, op1GPR, GPRInfo::tagTypeNumberRegister));
+ if (node->op() == ForwardInt32ToDouble) {
+ forwardSpeculationCheck(
+ BadType, JSValueRegs(op1GPR), node->child1().node(),
+ m_jit.branchTest64(MacroAssembler::Zero, op1GPR, GPRInfo::tagTypeNumberRegister),
+ ValueRecovery::inGPR(op1GPR, DataFormatJS));
+ } else {
+ speculationCheck(
+ BadType, JSValueRegs(op1GPR), node->child1(),
+ m_jit.branchTest64(MacroAssembler::Zero, op1GPR, GPRInfo::tagTypeNumberRegister));
+ }
}
m_jit.move(op1GPR, tempGPR);
@@ -2419,9 +2426,16 @@
MacroAssembler::Equal, op1TagGPR, TrustedImm32(JSValue::Int32Tag));
if (!isNumberSpeculation(m_state.forNode(node->child1()).m_type)) {
- speculationCheck(
- BadType, JSValueRegs(op1TagGPR, op1PayloadGPR), node->child1(),
- m_jit.branch32(MacroAssembler::AboveOrEqual, op1TagGPR, TrustedImm32(JSValue::LowestTag)));
+ if (node->op() == ForwardInt32ToDouble) {
+ forwardSpeculationCheck(
+ BadType, JSValueRegs(op1TagGPR, op1PayloadGPR), node->child1().node(),
+ m_jit.branch32(MacroAssembler::AboveOrEqual, op1TagGPR, TrustedImm32(JSValue::LowestTag)),
+ ValueRecovery::inPair(op1TagGPR, op1PayloadGPR));
+ } else {
+ speculationCheck(
+ BadType, JSValueRegs(op1TagGPR, op1PayloadGPR), node->child1(),
+ m_jit.branch32(MacroAssembler::AboveOrEqual, op1TagGPR, TrustedImm32(JSValue::LowestTag)));
+ }
}
unboxDouble(op1TagGPR, op1PayloadGPR, resultFPR, tempFPR);
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
index 0f053f7..8cc73d6 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
@@ -54,7 +54,6 @@
class SpeculateBooleanOperand;
enum GeneratedOperandType { GeneratedOperandTypeUnknown, GeneratedOperandInteger, GeneratedOperandDouble, GeneratedOperandJSValue};
-enum SpeculationDirection { ForwardSpeculation, BackwardSpeculation };
// === SpeculativeJIT ===
//
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
index ddef58d..f76bc53 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
@@ -2372,7 +2372,8 @@
break;
}
- case Int32ToDouble: {
+ case Int32ToDouble:
+ case ForwardInt32ToDouble: {
compileInt32ToDouble(node);
break;
}
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
index 59bac16..449fc47 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
@@ -2342,7 +2342,8 @@
break;
}
- case Int32ToDouble: {
+ case Int32ToDouble:
+ case ForwardInt32ToDouble: {
compileInt32ToDouble(node);
break;
}
diff --git a/Source/JavaScriptCore/dfg/DFGVariableAccessData.h b/Source/JavaScriptCore/dfg/DFGVariableAccessData.h
index 6d8e897..2128681 100644
--- a/Source/JavaScriptCore/dfg/DFGVariableAccessData.h
+++ b/Source/JavaScriptCore/dfg/DFGVariableAccessData.h
@@ -177,8 +177,12 @@
// If the variable is not a number prediction, then this doesn't
// make any sense.
- if (!isNumberSpeculation(prediction()))
+ if (!isNumberSpeculation(prediction())) {
+ // FIXME: we may end up forcing a local in inlined argument position to be a double even
+ // if it is sometimes not even numeric, since this never signals the fact that it doesn't
+ // want doubles. https://bugs.webkit.org/show_bug.cgi?id=109511
return false;
+ }
// If the variable is predicted to hold only doubles, then it's a
// no-brainer: it should be formatted as a double.
diff --git a/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp b/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp
index ee6a15d..300fe59 100644
--- a/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp
+++ b/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp
@@ -243,6 +243,7 @@
continue;
switch (node->op()) {
case Int32ToDouble:
+ case ForwardInt32ToDouble:
int32ToDoubleID = id;
break;
case ValueToInt32:
