DFG::StrCat isn't really effectful
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Aug 2015 23:59:57 +0000 (23:59 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Aug 2015 23:59:57 +0000 (23:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148443

Reviewed by Geoffrey Garen.

I previously made the DFG StrCat node effectful because it is implemented by calling a
DFGOperations function that could cause arbitrary effects. But, the node is only generated from the
op_strcat bytecode operation, and that operation is only used when we first ensure that its
operands are primitives. Primitive operands to StrCat cannot cause arbitrary side-effects. The
reason why I didn't immediately mark StrCat as pure was because there was nothing in DFG IR that
guaranteed that StrCat's children were primitives.

This change adds a KnownPrimitiveUse use kind, and applies it to StrCat. This allows us to mark
StrCat as being pure. This should be a speed-up because we can CSE StrCat and because it means that
we can OSR exit after a StrCat (a pure node doesn't clobber exit state), so we can convert more
of a large string concatenation into MakeRope's.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
* dfg/DFGOperations.cpp:
* dfg/DFGSafeToExecute.h:
(JSC::DFG::SafeToExecuteEdge::operator()):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::speculate):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGUseKind.cpp:
(WTF::printInternal):
* dfg/DFGUseKind.h:
(JSC::DFG::typeFilterFor):
(JSC::DFG::shouldNotHaveTypeCheck):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileStrCat):
(JSC::FTL::DFG::LowerDFGToLLVM::speculate):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189075 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/dfg/DFGUseKind.cpp
Source/JavaScriptCore/dfg/DFGUseKind.h
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

index fe2a009..46e394a 100644 (file)
@@ -1,3 +1,49 @@
+2015-08-27  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG::StrCat isn't really effectful
+        https://bugs.webkit.org/show_bug.cgi?id=148443
+
+        Reviewed by Geoffrey Garen.
+
+        I previously made the DFG StrCat node effectful because it is implemented by calling a
+        DFGOperations function that could cause arbitrary effects. But, the node is only generated from the
+        op_strcat bytecode operation, and that operation is only used when we first ensure that its
+        operands are primitives. Primitive operands to StrCat cannot cause arbitrary side-effects. The
+        reason why I didn't immediately mark StrCat as pure was because there was nothing in DFG IR that
+        guaranteed that StrCat's children were primitives.
+
+        This change adds a KnownPrimitiveUse use kind, and applies it to StrCat. This allows us to mark
+        StrCat as being pure. This should be a speed-up because we can CSE StrCat and because it means that
+        we can OSR exit after a StrCat (a pure node doesn't clobber exit state), so we can convert more
+        of a large string concatenation into MakeRope's.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
+        * dfg/DFGOperations.cpp:
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::SafeToExecuteEdge::operator()):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::speculate):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGUseKind.cpp:
+        (WTF::printInternal):
+        * dfg/DFGUseKind.h:
+        (JSC::DFG::typeFilterFor):
+        (JSC::DFG::shouldNotHaveTypeCheck):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileStrCat):
+        (JSC::FTL::DFG::LowerDFGToLLVM::speculate):
+
 2015-08-27  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Unreviewed build fix after r189064.
index 043e8f1..c49f375 100644 (file)
@@ -420,7 +420,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     }
 
     case StrCat: {
-        clobberWorld(node->origin.semantic, clobberLimit);
         forNode(node).setType(m_graph, SpecString);
         break;
     }
@@ -1575,7 +1574,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         
         clobberWorld(node->origin.semantic, clobberLimit);
         
-        forNode(node).setType(m_graph, (SpecHeapTop & ~SpecCell) | SpecString | SpecSymbol);
+        forNode(node).setType(m_graph, SpecHeapTop & ~SpecObject);
         break;
     }
         
index 6cd6992..67891b8 100644 (file)
@@ -157,6 +157,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
     case BooleanToNumber:
     case FiatInt52:
     case MakeRope:
+    case StrCat:
     case ValueToInt32:
     case GetExecutable:
     case BottomValue:
@@ -393,15 +394,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         write(Heap);
         return;
         
-    case StrCat:
-        // This is pretty weird. In fact, StrCat has very limited effectfulness because we only
-        // pass it primitive values. But, right now, the compiler isn't smart enough to know this
-        // and that's probably OK.
-        // FIXME: https://bugs.webkit.org/show_bug.cgi?id=148443
-        read(World);
-        write(Heap);
-        return;
-
     case GetGetter:
         read(GetterSetter_getter);
         def(HeapLocation(GetterLoc, GetterSetter_getter, node->child1()), LazyNode(node));
index ad6335a..bda4351 100644 (file)
@@ -160,7 +160,18 @@ private:
         }
 
         case StrCat: {
-            attemptToMakeFastStringAdd(node);
+            if (attemptToMakeFastStringAdd(node))
+                break;
+
+            // FIXME: Remove empty string arguments and possibly turn this into a ToString operation. That
+            // would require a form of ToString that takes a KnownPrimitiveUse. This is necessary because
+            // the implementation of StrCat doesn't dynamically optimize for empty strings.
+            // https://bugs.webkit.org/show_bug.cgi?id=148540
+            m_graph.doToChildren(
+                node,
+                [&] (Edge& edge) {
+                    fixEdge<KnownPrimitiveUse>(edge);
+                });
             break;
         }
             
@@ -1510,14 +1521,6 @@ private:
 
     bool attemptToMakeFastStringAdd(Node* node)
     {
-        if (!node->origin.exitOK) {
-            // If this code cannot exit, then we should not convert it to a MakeRope, since MakeRope
-            // can exit. This arises because we think that StrCat clobbers exit state, even though it
-            // doesn't really do that.
-            // FIXME: https://bugs.webkit.org/show_bug.cgi?id=148443
-            return false;
-        }
-        
         bool goodToGo = true;
         m_graph.doToChildren(
             node,
index 61d618e..1328e8e 100644 (file)
@@ -1114,11 +1114,9 @@ JSCell* JIT_OPERATION operationStrCat2(ExecState* exec, EncodedJSValue a, Encode
     NativeCallFrameTracer tracer(&vm, exec);
 
     JSString* str1 = JSValue::decode(a).toString(exec);
-    if (exec->hadException())
-        return nullptr;
+    ASSERT(!exec->hadException()); // Impossible, since we must have been given primitives.
     JSString* str2 = JSValue::decode(b).toString(exec);
-    if (exec->hadException())
-        return nullptr;
+    ASSERT(!exec->hadException());
 
     if (sumOverflows<int32_t>(str1->length(), str2->length())) {
         throwOutOfMemoryError(exec);
@@ -1134,14 +1132,11 @@ JSCell* JIT_OPERATION operationStrCat3(ExecState* exec, EncodedJSValue a, Encode
     NativeCallFrameTracer tracer(&vm, exec);
 
     JSString* str1 = JSValue::decode(a).toString(exec);
-    if (exec->hadException())
-        return nullptr;
+    ASSERT(!exec->hadException()); // Impossible, since we must have been given primitives.
     JSString* str2 = JSValue::decode(b).toString(exec);
-    if (exec->hadException())
-        return nullptr;
+    ASSERT(!exec->hadException());
     JSString* str3 = JSValue::decode(c).toString(exec);
-    if (exec->hadException())
-        return nullptr;
+    ASSERT(!exec->hadException());
 
     if (sumOverflows<int32_t>(str1->length(), str2->length(), str3->length())) {
         throwOutOfMemoryError(exec);
index 6980821..7f36616 100644 (file)
@@ -89,6 +89,11 @@ public:
             if (m_state.forNode(edge).m_type & ~SpecString)
                 m_result = false;
             return;
+
+        case KnownPrimitiveUse:
+            if (m_state.forNode(edge).m_type & ~(SpecHeapTop & ~SpecObject))
+                m_result = false;
+            return;
             
         case LastUseKind:
             RELEASE_ASSERT_NOT_REACHED();
index cee6bef..29ede52 100644 (file)
@@ -5881,6 +5881,9 @@ void SpeculativeJIT::speculate(Node*, Edge edge)
     case KnownStringUse:
         ASSERT(!needsTypeCheck(edge, SpecString));
         break;
+    case KnownPrimitiveUse:
+        ASSERT(!needsTypeCheck(edge, SpecHeapTop & ~SpecObject));
+        break;
     case Int32Use:
         speculateInt32(edge);
         break;
index 51c751c..515c063 100644 (file)
@@ -2063,9 +2063,9 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case StrCat: {
-        JSValueOperand op1(this, node->child1());
-        JSValueOperand op2(this, node->child2());
-        JSValueOperand op3(this, node->child3());
+        JSValueOperand op1(this, node->child1(), ManualOperandSpeculation);
+        JSValueOperand op2(this, node->child2(), ManualOperandSpeculation);
+        JSValueOperand op3(this, node->child3(), ManualOperandSpeculation);
         
         GPRReg op1TagGPR = op1.tagGPR();
         GPRReg op1PayloadGPR = op1.payloadGPR();
index 1f17527..379d9c3 100644 (file)
@@ -2195,9 +2195,9 @@ void SpeculativeJIT::compile(Node* node)
     }
         
     case StrCat: {
-        JSValueOperand op1(this, node->child1());
-        JSValueOperand op2(this, node->child2());
-        JSValueOperand op3(this, node->child3());
+        JSValueOperand op1(this, node->child1(), ManualOperandSpeculation);
+        JSValueOperand op2(this, node->child2(), ManualOperandSpeculation);
+        JSValueOperand op3(this, node->child3(), ManualOperandSpeculation);
         
         GPRReg op1GPR = op1.gpr();
         GPRReg op2GPR = op2.gpr();
index 3442341..ad4f26e 100644 (file)
@@ -100,6 +100,9 @@ void printInternal(PrintStream& out, UseKind useKind)
     case KnownStringUse:
         out.print("KnownString");
         return;
+    case KnownPrimitiveUse:
+        out.print("KnownPrimitive");
+        return;
     case SymbolUse:
         out.print("Symbol");
         return;
index 800dcbb..d82fc1a 100644 (file)
@@ -58,6 +58,7 @@ enum UseKind {
     StringIdentUse,
     StringUse,
     KnownStringUse,
+    KnownPrimitiveUse, // This bizarre type arises for op_strcat, which has a bytecode guarantee that it will only see primitives (i.e. not objects).
     SymbolUse,
     StringObjectUse,
     StringOrStringObjectUse,
@@ -120,6 +121,8 @@ inline SpeculatedType typeFilterFor(UseKind useKind)
     case StringUse:
     case KnownStringUse:
         return SpecString;
+    case KnownPrimitiveUse:
+        return SpecHeapTop & ~SpecObject;
     case SymbolUse:
         return SpecSymbol;
     case StringObjectUse:
@@ -147,6 +150,7 @@ inline bool shouldNotHaveTypeCheck(UseKind kind)
     case KnownInt32Use:
     case KnownCellUse:
     case KnownStringUse:
+    case KnownPrimitiveUse:
     case KnownBooleanUse:
     case Int52RepUse:
     case DoubleRepUse:
index ea16b07..80304a3 100644 (file)
@@ -422,6 +422,7 @@ CapabilityLevel canCompile(Graph& graph)
                 case ObjectOrOtherUse:
                 case StringUse:
                 case KnownStringUse:
+                case KnownPrimitiveUse:
                 case StringObjectUse:
                 case StringOrStringObjectUse:
                 case SymbolUse:
index 4fdf180..55a368d 100644 (file)
@@ -1326,12 +1326,14 @@ private:
         if (m_node->child3()) {
             result = vmCall(
                 m_out.operation(operationStrCat3), m_callFrame,
-                lowJSValue(m_node->child1()), lowJSValue(m_node->child2()),
-                lowJSValue(m_node->child3()));
+                lowJSValue(m_node->child1(), ManualOperandSpeculation),
+                lowJSValue(m_node->child2(), ManualOperandSpeculation),
+                lowJSValue(m_node->child3(), ManualOperandSpeculation));
         } else {
             result = vmCall(
                 m_out.operation(operationStrCat2), m_callFrame,
-                lowJSValue(m_node->child1()), lowJSValue(m_node->child2()));
+                lowJSValue(m_node->child1(), ManualOperandSpeculation),
+                lowJSValue(m_node->child2(), ManualOperandSpeculation));
         }
         setJSValue(result);
     }
@@ -7495,6 +7497,7 @@ private:
             break;
         case KnownInt32Use:
         case KnownStringUse:
+        case KnownPrimitiveUse:
         case DoubleRepUse:
         case Int52RepUse:
             ASSERT(!m_interpreter.needsTypeCheck(edge));