From ef85ec08e408d723c2de36efb3c3ee2f9611b851 Mon Sep 17 00:00:00 2001 From: "utatane.tea@gmail.com" Date: Thu, 16 Mar 2017 04:49:47 +0000 Subject: [PATCH] [DFG] ToString operation should have fixup for primitives to say this node does not have side effects https://bugs.webkit.org/show_bug.cgi?id=169544 Reviewed by Saam Barati. JSTests: * microbenchmarks/template-string-array.js: Added. (test): * stress/to-string-non-cell-use.js: Added. (shouldBe): (shouldThrow): Source/JavaScriptCore: Our DFG ToString only considers well about String operands. While ToString(non cell operand) does not have any side effect, it is not modeled well in DFG. This patch introduces a fixup for ToString with NonCellUse edge. If this edge is set, ToString does not clobber things (like ToLowerCase, producing String). And ToString(NonCellUse) allows us to perform CSE! Our microbenchmark shows 32.9% improvement due to dropped GetButterfly and CSE for ToString(). baseline patched template-string-array 12.6284+-0.2766 ^ 9.4998+-0.2295 ^ definitely 1.3293x faster And SixSpeed template_string.es6 shows 16.68x performance improvement due to LICM onto this non-side-effectful ToString(). baseline patched template_string.es6 3229.7343+-40.5705 ^ 193.6077+-36.3349 ^ definitely 16.6818x faster * dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter::executeEffects): * dfg/DFGClobberize.h: (JSC::DFG::clobberize): * dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor): * dfg/DFGSpeculativeJIT.cpp: (JSC::DFG::SpeculativeJIT::compileToStringOrCallStringConstructorOnCell): (JSC::DFG::SpeculativeJIT::speculateNotCell): * dfg/DFGSpeculativeJIT.h: * dfg/DFGSpeculativeJIT32_64.cpp: (JSC::DFG::SpeculativeJIT::compile): * dfg/DFGSpeculativeJIT64.cpp: (JSC::DFG::SpeculativeJIT::compile): * ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::compileToStringOrCallStringConstructor): (JSC::FTL::DFG::LowerDFGToB3::lowNotCell): (JSC::FTL::DFG::LowerDFGToB3::speculateNotCell): git-svn-id: https://svn.webkit.org/repository/webkit/trunk@214028 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- JSTests/ChangeLog | 13 +++++ JSTests/microbenchmarks/template-string-array.js | 9 ++++ JSTests/stress/to-string-non-cell-use.js | 43 +++++++++++++++ Source/JavaScriptCore/ChangeLog | 44 +++++++++++++++ .../dfg/DFGAbstractInterpreterInlines.h | 1 + Source/JavaScriptCore/dfg/DFGClobberize.h | 4 ++ Source/JavaScriptCore/dfg/DFGFixupPhase.cpp | 10 ++++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp | 62 +++++++++++++++++++++- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h | 6 +++ .../JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp | 32 ----------- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp | 31 ----------- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp | 28 ++++++---- 12 files changed, 210 insertions(+), 73 deletions(-) create mode 100644 JSTests/microbenchmarks/template-string-array.js create mode 100644 JSTests/stress/to-string-non-cell-use.js diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog index becefe3a..1fb2317 100644 --- a/JSTests/ChangeLog +++ b/JSTests/ChangeLog @@ -1,3 +1,16 @@ +2017-03-15 Yusuke Suzuki + + [DFG] ToString operation should have fixup for primitives to say this node does not have side effects + https://bugs.webkit.org/show_bug.cgi?id=169544 + + Reviewed by Saam Barati. + + * microbenchmarks/template-string-array.js: Added. + (test): + * stress/to-string-non-cell-use.js: Added. + (shouldBe): + (shouldThrow): + 2017-03-13 Commit Queue Unreviewed, rolling out r213856. diff --git a/JSTests/microbenchmarks/template-string-array.js b/JSTests/microbenchmarks/template-string-array.js new file mode 100644 index 0000000..88b669c --- /dev/null +++ b/JSTests/microbenchmarks/template-string-array.js @@ -0,0 +1,9 @@ +var array = [1, 2, 3]; +function test() +{ + return `${array[0]}, ${array[1]}, ${array[2]}, ${array[0]}, ${array[1]}, ${array[2]}`; +} +noInline(test); + +for (var i = 0; i < 1e5; ++i) + test(); diff --git a/JSTests/stress/to-string-non-cell-use.js b/JSTests/stress/to-string-non-cell-use.js new file mode 100644 index 0000000..573cd56 --- /dev/null +++ b/JSTests/stress/to-string-non-cell-use.js @@ -0,0 +1,43 @@ +function shouldBe(actual, expected) +{ + if (actual !== expected) + throw new Error('bad value: ' + actual); +} + +function shouldThrow(func, errorMessage) +{ + var errorThrown = false; + var error = null; + try { + func(); + } catch (e) { + errorThrown = true; + error = e; + } + if (!errorThrown) + throw new Error('not thrown'); + if (String(error) !== errorMessage) + throw new Error(`bad error: ${String(error)}`); +} + +function toString(value) +{ + return `${value}`; +} +noInline(toString); + +for (var i = 0; i < 1e4; ++i) { + shouldBe(toString(i), i + ""); + shouldBe(toString(null), "null"); + shouldBe(toString(undefined), "undefined"); + shouldBe(toString(10.5), "10.5"); + shouldBe(toString(-10.5), "-10.5"); + shouldBe(toString(true), "true"); + shouldBe(toString(false), "false"); + shouldBe(toString(0 / 0), "NaN"); +} + +shouldBe(toString("HELLO"), "HELLO"); +shouldThrow(() => { + toString(Symbol("Cocoa")); +}, `TypeError: Cannot convert a symbol to a string`); diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index 91d5ea9..daff8c4 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,47 @@ +2017-03-15 Yusuke Suzuki + + [DFG] ToString operation should have fixup for primitives to say this node does not have side effects + https://bugs.webkit.org/show_bug.cgi?id=169544 + + Reviewed by Saam Barati. + + Our DFG ToString only considers well about String operands. While ToString(non cell operand) does not have + any side effect, it is not modeled well in DFG. + + This patch introduces a fixup for ToString with NonCellUse edge. If this edge is set, ToString does not + clobber things (like ToLowerCase, producing String). And ToString(NonCellUse) allows us to perform CSE! + + Our microbenchmark shows 32.9% improvement due to dropped GetButterfly and CSE for ToString(). + + baseline patched + + template-string-array 12.6284+-0.2766 ^ 9.4998+-0.2295 ^ definitely 1.3293x faster + + And SixSpeed template_string.es6 shows 16.68x performance improvement due to LICM onto this non-side-effectful ToString(). + + baseline patched + + template_string.es6 3229.7343+-40.5705 ^ 193.6077+-36.3349 ^ definitely 16.6818x faster + + * dfg/DFGAbstractInterpreterInlines.h: + (JSC::DFG::AbstractInterpreter::executeEffects): + * dfg/DFGClobberize.h: + (JSC::DFG::clobberize): + * dfg/DFGFixupPhase.cpp: + (JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor): + * dfg/DFGSpeculativeJIT.cpp: + (JSC::DFG::SpeculativeJIT::compileToStringOrCallStringConstructorOnCell): + (JSC::DFG::SpeculativeJIT::speculateNotCell): + * dfg/DFGSpeculativeJIT.h: + * dfg/DFGSpeculativeJIT32_64.cpp: + (JSC::DFG::SpeculativeJIT::compile): + * dfg/DFGSpeculativeJIT64.cpp: + (JSC::DFG::SpeculativeJIT::compile): + * ftl/FTLLowerDFGToB3.cpp: + (JSC::FTL::DFG::LowerDFGToB3::compileToStringOrCallStringConstructor): + (JSC::FTL::DFG::LowerDFGToB3::lowNotCell): + (JSC::FTL::DFG::LowerDFGToB3::speculateNotCell): + 2017-03-15 Ryan Haddad Revert part of r213978 to see if it resolves LayoutTest crashes. diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h index 0d2e929..fbfe166 100644 --- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h +++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h @@ -1858,6 +1858,7 @@ bool AbstractInterpreter::executeEffects(unsigned clobberLimi m_graph.registerStructure(m_graph.globalObjectFor(node->origin.semantic)->stringObjectStructure())); break; case StringOrStringObjectUse: + case NotCellUse: break; case CellUse: case UntypedUse: diff --git a/Source/JavaScriptCore/dfg/DFGClobberize.h b/Source/JavaScriptCore/dfg/DFGClobberize.h index 4a665a3..d2e372e 100644 --- a/Source/JavaScriptCore/dfg/DFGClobberize.h +++ b/Source/JavaScriptCore/dfg/DFGClobberize.h @@ -1408,6 +1408,10 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu read(World); write(Heap); return; + + case NotCellUse: + def(PureValue(node)); + return; default: RELEASE_ASSERT_NOT_REACHED(); diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp index 89b9046..9904e73 100644 --- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp @@ -2235,6 +2235,16 @@ private: fixEdge(node->child1()); return; } + + // ToString(Symbol) throws an error. So if the child1 can include Symbols, + // we need to care about it in the clobberize. In the following case, + // since NotCellUse edge filter is used and this edge filters Symbols, + // we can say that ToString never throws an error! + if (node->child1()->shouldSpeculateNotCell()) { + fixEdge(node->child1()); + node->clearFlags(NodeMustGenerate); + return; + } } bool attemptToMakeFastStringAdd(Node* node) diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp index fbaef3b..07c82c4 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp @@ -7925,6 +7925,61 @@ GPRReg SpeculativeJIT::temporaryRegisterForPutByVal(GPRTemporary& temporary, Arr void SpeculativeJIT::compileToStringOrCallStringConstructorOnCell(Node* node) { + if (node->child1().useKind() == NotCellUse) { + JSValueOperand op1(this, node->child1(), ManualOperandSpeculation); + JSValueRegs op1Regs = op1.jsValueRegs(); + + GPRFlushedCallResult result(this); + GPRReg resultGPR = result.gpr(); + + speculateNotCell(node->child1(), op1Regs); + + flushRegisters(); + + if (node->op() == ToString) + callOperation(operationToString, resultGPR, op1Regs); + else { + ASSERT(node->op() == CallStringConstructor); + callOperation(operationCallStringConstructor, resultGPR, op1Regs); + } + m_jit.exceptionCheck(); + cellResult(resultGPR, node); + return; + } + + if (node->child1().useKind() == UntypedUse) { + JSValueOperand op1(this, node->child1()); + JSValueRegs op1Regs = op1.jsValueRegs(); + GPRReg op1PayloadGPR = op1Regs.payloadGPR(); + + GPRFlushedCallResult result(this); + GPRReg resultGPR = result.gpr(); + + flushRegisters(); + + JITCompiler::Jump done; + if (node->child1()->prediction() & SpecString) { + JITCompiler::Jump slowPath1 = m_jit.branchIfNotCell(op1.jsValueRegs()); + JITCompiler::Jump slowPath2 = m_jit.branchIfNotString(op1PayloadGPR); + m_jit.move(op1PayloadGPR, resultGPR); + done = m_jit.jump(); + slowPath1.link(&m_jit); + slowPath2.link(&m_jit); + } + if (node->op() == ToString) + callOperation(operationToString, resultGPR, op1Regs); + else { + ASSERT(node->op() == CallStringConstructor); + callOperation(operationCallStringConstructor, resultGPR, op1Regs); + } + m_jit.exceptionCheck(); + if (done.isSet()) + done.link(&m_jit); + cellResult(resultGPR, node); + return; + } + + SpeculateCellOperand op1(this, node->child1()); GPRReg op1GPR = op1.gpr(); @@ -8533,13 +8588,18 @@ void SpeculativeJIT::speculateSymbol(Edge edge) speculateSymbol(edge, operand.gpr()); } +void SpeculativeJIT::speculateNotCell(Edge edge, JSValueRegs regs) +{ + DFG_TYPE_CHECK(regs, edge, ~SpecCell, m_jit.branchIfCell(regs)); +} + void SpeculativeJIT::speculateNotCell(Edge edge) { if (!needsTypeCheck(edge, ~SpecCell)) return; JSValueOperand operand(this, edge, ManualOperandSpeculation); - typeCheck(operand.jsValueRegs(), edge, ~SpecCell, m_jit.branchIfCell(operand.jsValueRegs())); + speculateNotCell(edge, operand.jsValueRegs()); } void SpeculativeJIT::speculateOther(Edge edge) diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h index e9637fb..e17e447 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h @@ -1644,6 +1644,11 @@ public: m_jit.setupArgumentsWithExecState(arg1); return appendCallSetResult(operation, result); } + JITCompiler::Call callOperation(C_JITOperation_EJ operation, GPRReg result, JSValueRegs arg1) + { + m_jit.setupArgumentsWithExecState(arg1.gpr()); + return appendCallSetResult(operation, result); + } JITCompiler::Call callOperation(C_JITOperation_EJJ operation, GPRReg result, GPRReg arg1, GPRReg arg2) { m_jit.setupArgumentsWithExecState(arg1, arg2); @@ -2954,6 +2959,7 @@ public: void speculateStringOrStringObject(Edge); void speculateSymbol(Edge, GPRReg cell); void speculateSymbol(Edge); + void speculateNotCell(Edge, JSValueRegs); void speculateNotCell(Edge); void speculateOther(Edge); void speculateMisc(Edge, JSValueRegs); diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp index 7356122..dc76334 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp @@ -3769,38 +3769,6 @@ void SpeculativeJIT::compile(Node* node) case ToString: case CallStringConstructor: { - if (node->child1().useKind() == UntypedUse) { - JSValueOperand op1(this, node->child1()); - GPRReg op1PayloadGPR = op1.payloadGPR(); - JSValueRegs op1Regs = op1.jsValueRegs(); - - GPRFlushedCallResult result(this); - GPRReg resultGPR = result.gpr(); - - flushRegisters(); - - JITCompiler::Jump done; - if (node->child1()->prediction() & SpecString) { - JITCompiler::Jump slowPath1 = m_jit.branchIfNotCell(op1.jsValueRegs()); - JITCompiler::Jump slowPath2 = m_jit.branchIfNotString(op1PayloadGPR); - m_jit.move(op1PayloadGPR, resultGPR); - done = m_jit.jump(); - slowPath1.link(&m_jit); - slowPath2.link(&m_jit); - } - if (op == ToString) - callOperation(operationToString, resultGPR, op1Regs); - else { - ASSERT(op == CallStringConstructor); - callOperation(operationCallStringConstructor, resultGPR, op1Regs); - } - m_jit.exceptionCheck(); - if (done.isSet()) - done.link(&m_jit); - cellResult(resultGPR, node); - break; - } - compileToStringOrCallStringConstructorOnCell(node); break; } diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp index a05d8db..a53a77a 100644 --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp @@ -3739,37 +3739,6 @@ void SpeculativeJIT::compile(Node* node) case ToString: case CallStringConstructor: { - if (node->child1().useKind() == UntypedUse) { - JSValueOperand op1(this, node->child1()); - GPRReg op1GPR = op1.gpr(); - - GPRFlushedCallResult result(this); - GPRReg resultGPR = result.gpr(); - - flushRegisters(); - - JITCompiler::Jump done; - if (node->child1()->prediction() & SpecString) { - JITCompiler::Jump slowPath1 = m_jit.branchIfNotCell(JSValueRegs(op1GPR)); - JITCompiler::Jump slowPath2 = m_jit.branchIfNotString(op1GPR); - m_jit.move(op1GPR, resultGPR); - done = m_jit.jump(); - slowPath1.link(&m_jit); - slowPath2.link(&m_jit); - } - if (op == ToString) - callOperation(operationToString, resultGPR, op1GPR); - else { - ASSERT(op == CallStringConstructor); - callOperation(operationCallStringConstructor, resultGPR, op1GPR); - } - m_jit.exceptionCheck(); - if (done.isSet()) - done.link(&m_jit); - cellResult(resultGPR, node); - break; - } - compileToStringOrCallStringConstructorOnCell(node); break; } diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp index 6c9885f..9719136 100644 --- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp +++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp @@ -4933,10 +4933,13 @@ private: } case CellUse: + case NotCellUse: case UntypedUse: { LValue value; if (m_node->child1().useKind() == CellUse) value = lowCell(m_node->child1()); + else if (m_node->child1().useKind() == NotCellUse) + value = lowNotCell(m_node->child1()); else value = lowJSValue(m_node->child1()); @@ -4947,6 +4950,8 @@ private: LValue isCellPredicate; if (m_node->child1().useKind() == CellUse) isCellPredicate = m_out.booleanTrue; + else if (m_node->child1().useKind() == NotCellUse) + isCellPredicate = m_out.booleanFalse; else isCellPredicate = this->isCell(value, provenType(m_node->child1())); m_out.branch(isCellPredicate, unsure(isCell), unsure(notString)); @@ -12228,6 +12233,13 @@ private: DFG_CRASH(m_graph, m_node, "Value not defined"); return 0; } + + LValue lowNotCell(Edge edge) + { + LValue result = lowJSValue(edge, ManualOperandSpeculation); + FTL_TYPE_CHECK(jsValueValue(result), edge, ~SpecCell, isCell(result)); + return result; + } LValue lowStorage(Edge edge) { @@ -12632,6 +12644,13 @@ private: { lowCell(edge); } + + void speculateNotCell(Edge edge) + { + if (!m_interpreter.needsTypeCheck(edge)) + return; + lowNotCell(edge); + } void speculateCellOrOther(Edge edge) { @@ -13150,15 +13169,6 @@ private: m_out.appendTo(continuation, lastNext); } - void speculateNotCell(Edge edge) - { - if (!m_interpreter.needsTypeCheck(edge)) - return; - - LValue value = lowJSValue(edge, ManualOperandSpeculation); - typeCheck(jsValueValue(value), edge, ~SpecCell, isCell(value)); - } - void speculateOther(Edge edge) { if (!m_interpreter.needsTypeCheck(edge)) -- 1.8.3.1