- Revision
- 153778
- Author
- [email protected]
- Date
- 2013-08-06 21:04:50 -0700 (Tue, 06 Aug 2013)
Log Message
DFG FixupPhase should insert Int32ToDouble nodes for number uses in NewArray, and SpeculativeJIT 64-bit should not try to coerce integer constants to double constants
https://bugs.webkit.org/show_bug.cgi?id=119528
Reviewed by Geoffrey Garen.
Source/_javascript_Core:
Either of the two fixes would solve the crash I saw. Basically, for best performance, we want the DFG register allocator to track double uses and non-double
uses of a node separately, and we accomplish this by inserting Int32ToDouble nodes in the FixupPhase. But even if FixupPhase fails to do this, we still want
the DFG register allocator to do the right thing: if it encounters a double use of an integer, it should perform a conversion and preserve the original
format of the value (namely, that it was an integer). For constants, the best format to preserve is None, so that future integer uses rematerialize the int
from scratch. This only affects the 64-bit backend; the 32-bit backend was already doing the right thing.
This also fixes some more debug dumping code, and adds some stronger assertions for integer arrays.
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::finalizeUnconditionally):
* dfg/DFGDriver.cpp:
(JSC::DFG::compile):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::dump):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::fillSpeculateDouble):
* runtime/JSObject.h:
(JSC::JSObject::getIndexQuickly):
(JSC::JSObject::tryGetIndexQuickly):
LayoutTests:
* fast/js/dfg-new-array-double-const-then-int-const.html: Added.
* fast/js/dfg-new-array-double-const-then-int-const-expected.txt: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-new-array-double-const-then-int-const.js: Added.
(bar):
(foo):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (153777 => 153778)
--- trunk/LayoutTests/ChangeLog 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/LayoutTests/ChangeLog 2013-08-07 04:04:50 UTC (rev 153778)
@@ -1,3 +1,17 @@
+2013-08-06 Filip Pizlo <[email protected]>
+
+ DFG FixupPhase should insert Int32ToDouble nodes for number uses in NewArray, and SpeculativeJIT 64-bit should not try to coerce integer constants to double constants
+ https://bugs.webkit.org/show_bug.cgi?id=119528
+
+ Reviewed by Geoffrey Garen.
+
+ * fast/js/dfg-new-array-double-const-then-int-const.html: Added.
+ * fast/js/dfg-new-array-double-const-then-int-const-expected.txt: Added.
+ * fast/js/jsc-test-list:
+ * fast/js/script-tests/dfg-new-array-double-const-then-int-const.js: Added.
+ (bar):
+ (foo):
+
2013-08-04 Sam Weinig <[email protected]>
Remove support for HTML5 MicroData
Added: trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const-expected.txt (0 => 153778)
--- trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const-expected.txt 2013-08-07 04:04:50 UTC (rev 153778)
@@ -0,0 +1,9 @@
+Tests what happens when you have a NewArray that uses an integer constant as a double constant, and then that constant is subsequently used as an integer constant.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Passed some tests silently.
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const.html (0 => 153778)
--- trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const.html (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-new-array-double-const-then-int-const.html 2013-08-07 04:04:50 UTC (rev 153778)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Modified: trunk/LayoutTests/fast/js/jsc-test-list (153777 => 153778)
--- trunk/LayoutTests/fast/js/jsc-test-list 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/LayoutTests/fast/js/jsc-test-list 2013-08-07 04:04:50 UTC (rev 153778)
@@ -199,6 +199,7 @@
fast/js/dfg-multi-basic-block-structure-clobber
fast/js/dfg-multiply
fast/js/dfg-negative-array-index
+fast/js/dfg-new-array-double-const-then-int-const
fast/js/dfg-obvious-constant-cfa
fast/js/dfg-osr-entry-hoisted-clobbered-structure-check
fast/js/dfg-other-branch
Added: trunk/LayoutTests/fast/js/script-tests/dfg-new-array-double-const-then-int-const.js (0 => 153778)
--- trunk/LayoutTests/fast/js/script-tests/dfg-new-array-double-const-then-int-const.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-new-array-double-const-then-int-const.js 2013-08-07 04:04:50 UTC (rev 153778)
@@ -0,0 +1,19 @@
+description(
+"Tests what happens when you have a NewArray that uses an integer constant as a double constant, and then that constant is subsequently used as an integer constant."
+);
+
+function bar(x) {
+ return [[x, 1.5], [x, 0]];
+}
+
+function foo() {
+ return bar(1)[1][0];
+}
+
+silentTestPass = true;
+noInline(foo);
+
+for (var i = 0; i < 2; i = dfgIncrement({f:foo, i:i + 1, n:1}))
+ shouldBe("foo()", "1");
+
+
Modified: trunk/Source/_javascript_Core/ChangeLog (153777 => 153778)
--- trunk/Source/_javascript_Core/ChangeLog 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-08-07 04:04:50 UTC (rev 153778)
@@ -1,3 +1,32 @@
+2013-08-06 Filip Pizlo <[email protected]>
+
+ DFG FixupPhase should insert Int32ToDouble nodes for number uses in NewArray, and SpeculativeJIT 64-bit should not try to coerce integer constants to double constants
+ https://bugs.webkit.org/show_bug.cgi?id=119528
+
+ Reviewed by Geoffrey Garen.
+
+ Either of the two fixes would solve the crash I saw. Basically, for best performance, we want the DFG register allocator to track double uses and non-double
+ uses of a node separately, and we accomplish this by inserting Int32ToDouble nodes in the FixupPhase. But even if FixupPhase fails to do this, we still want
+ the DFG register allocator to do the right thing: if it encounters a double use of an integer, it should perform a conversion and preserve the original
+ format of the value (namely, that it was an integer). For constants, the best format to preserve is None, so that future integer uses rematerialize the int
+ from scratch. This only affects the 64-bit backend; the 32-bit backend was already doing the right thing.
+
+ This also fixes some more debug dumping code, and adds some stronger assertions for integer arrays.
+
+ * bytecode/CodeBlock.cpp:
+ (JSC::CodeBlock::finalizeUnconditionally):
+ * dfg/DFGDriver.cpp:
+ (JSC::DFG::compile):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ * dfg/DFGGraph.cpp:
+ (JSC::DFG::Graph::dump):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::fillSpeculateDouble):
+ * runtime/JSObject.h:
+ (JSC::JSObject::getIndexQuickly):
+ (JSC::JSObject::tryGetIndexQuickly):
+
2013-08-06 Mark Lam <[email protected]>
Assertion failure in emitExpressionInfo when reloading with Web Inspector open.
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (153777 => 153778)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2013-08-07 04:04:50 UTC (rev 153778)
@@ -2206,13 +2206,13 @@
JSCell* to = transition.m_to.get();
if ((!origin || Heap::isMarked(origin)) && Heap::isMarked(from))
continue;
- dataLog(" Transition under ", JSValue(origin), ", ", JSValue(from), " -> ", JSValue(to), ".\n");
+ dataLog(" Transition under ", RawPointer(origin), ", ", RawPointer(from), " -> ", RawPointer(to), ".\n");
}
for (unsigned i = 0; i < dfgCommon->weakReferences.size(); ++i) {
JSCell* weak = dfgCommon->weakReferences[i].get();
if (Heap::isMarked(weak))
continue;
- dataLog(" Weak reference ", JSValue(weak), ".\n");
+ dataLog(" Weak reference ", RawPointer(weak), ".\n");
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGDriver.cpp (153777 => 153778)
--- trunk/Source/_javascript_Core/dfg/DFGDriver.cpp 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/dfg/DFGDriver.cpp 2013-08-07 04:04:50 UTC (rev 153778)
@@ -72,7 +72,6 @@
if (!Options::bytecodeRangeToDFGCompile().isInRange(codeBlock->instructionCount()))
return CompilationFailed;
-
if (logCompilationChanges())
dataLog("DFG(Driver) compiling ", *codeBlock, ", number of instructions = ", codeBlock->instructionCount(), "\n");
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (153777 => 153778)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2013-08-07 04:04:50 UTC (rev 153778)
@@ -661,7 +661,7 @@
break;
case ALL_DOUBLE_INDEXING_TYPES:
for (unsigned operandIndex = 0; operandIndex < node->numChildren(); ++operandIndex)
- setUseKindAndUnboxIfProfitable<RealNumberUse>(m_graph.m_varArgChildren[node->firstChild() + operandIndex]);
+ fixDoubleEdge<RealNumberUse>(m_graph.m_varArgChildren[node->firstChild() + operandIndex]);
break;
case ALL_CONTIGUOUS_INDEXING_TYPES:
case ALL_ARRAY_STORAGE_INDEXING_TYPES:
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.cpp (153777 => 153778)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2013-08-07 04:04:50 UTC (rev 153778)
@@ -216,11 +216,11 @@
}
if (node->hasFunctionDeclIndex()) {
FunctionExecutable* executable = m_codeBlock->functionDecl(node->functionDeclIndex());
- out.print(comma, executable->inferredName().string(), "#", executable->hashFor(CodeForCall));
+ out.print(comma, FunctionExecutableDump(executable));
}
if (node->hasFunctionExprIndex()) {
FunctionExecutable* executable = m_codeBlock->functionExpr(node->functionExprIndex());
- out.print(comma, executable->inferredName().string(), "#", executable->hashFor(CodeForCall));
+ out.print(comma, FunctionExecutableDump(executable));
}
if (node->hasStorageAccessData()) {
StorageAccessData& storageAccessData = m_storageAccessData[node->storageAccessDataIndex()];
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (153777 => 153778)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2013-08-07 04:04:50 UTC (rev 153778)
@@ -987,8 +987,9 @@
m_jit.move64ToDouble(gpr, fpr);
unlock(gpr);
- m_fprs.retain(fpr, virtualRegister, SpillOrderDouble);
- info.fillDouble(*m_stream, fpr);
+ // Don't fill double here since that will lead to confusion: the
+ // register allocator will now think that this is a double while
+ // everyone else thinks it's an integer.
return fpr;
}
if (isNumberConstant(edge.node())) {
Modified: trunk/Source/_javascript_Core/runtime/JSObject.h (153777 => 153778)
--- trunk/Source/_javascript_Core/runtime/JSObject.h 2013-08-07 03:07:28 UTC (rev 153777)
+++ trunk/Source/_javascript_Core/runtime/JSObject.h 2013-08-07 04:04:50 UTC (rev 153778)
@@ -223,6 +223,7 @@
{
switch (structure()->indexingType()) {
case ALL_INT32_INDEXING_TYPES:
+ return jsNumber(m_butterfly->contiguous()[i].get().asInt32());
case ALL_CONTIGUOUS_INDEXING_TYPES:
return m_butterfly->contiguous()[i].get();
case ALL_DOUBLE_INDEXING_TYPES:
@@ -242,6 +243,12 @@
case ALL_UNDECIDED_INDEXING_TYPES:
break;
case ALL_INT32_INDEXING_TYPES:
+ if (i < m_butterfly->publicLength()) {
+ JSValue result = m_butterfly->contiguous()[i].get();
+ ASSERT(result.isInt32() || !result);
+ return result;
+ }
+ break;
case ALL_CONTIGUOUS_INDEXING_TYPES:
if (i < m_butterfly->publicLength())
return m_butterfly->contiguous()[i].get();
