Remove ConditionalStore barrier
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Mar 2014 22:21:44 +0000 (22:21 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Mar 2014 22:21:44 +0000 (22:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=130040

Reviewed by Geoffrey Garen.

ConditionalStoreBarrier was created when barriers were much more expensive. Now that
they're cheap(er), we can get rid of them. This also allows us to get rid of the write
barrier logic in emitPutTransitionStub because we always will have executed a write barrier
on the base object in the case where we are allocating and storing a new Butterfly into it.
Previously, a ConditionalStoreBarrier might or might not have barrier-ed the base object,
so we'd have to emit a write barrier in the transition case.

This is performance neutral on the benchmarks we track.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
(JSC::DFG::ConstantFoldingPhase::emitPutByOffset):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::insertStoreBarrier):
* dfg/DFGNode.h:
(JSC::DFG::Node::isStoreBarrier):
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileStoreBarrier):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::compileNode):
* jit/Repatch.cpp:
(JSC::emitPutTransitionStub):

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

15 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/jit/Repatch.cpp

index d0f0629..f221501 100644 (file)
@@ -1,3 +1,49 @@
+2014-03-10  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        Remove ConditionalStore barrier
+        https://bugs.webkit.org/show_bug.cgi?id=130040
+
+        Reviewed by Geoffrey Garen.
+
+        ConditionalStoreBarrier was created when barriers were much more expensive. Now that 
+        they're cheap(er), we can get rid of them. This also allows us to get rid of the write 
+        barrier logic in emitPutTransitionStub because we always will have executed a write barrier 
+        on the base object in the case where we are allocating and storing a new Butterfly into it. 
+        Previously, a ConditionalStoreBarrier might or might not have barrier-ed the base object, 
+        so we'd have to emit a write barrier in the transition case.
+
+        This is performance neutral on the benchmarks we track.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+        (JSC::DFG::ConstantFoldingPhase::emitPutByOffset):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::insertStoreBarrier):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::isStoreBarrier):
+        * dfg/DFGNodeType.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        (JSC::DFG::PredictionPropagationPhase::propagate):
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::safeToExecute):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileStoreBarrier):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::compileNode):
+        * jit/Repatch.cpp:
+        (JSC::emitPutTransitionStub):
+
 2014-03-10  Filip Pizlo  <fpizlo@apple.com>
 
         DFG and FTL should know that comparing anything to Misc is cheap and easy
index 955d932..5049ff8 100644 (file)
@@ -1844,13 +1844,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     case CheckTierUpAtReturn:
         break;
 
-    case ConditionalStoreBarrier: {
-        if (!needsTypeCheck(node->child2().node(), ~SpecCell))
-            m_state.setFoundConstants(true);
-        filter(node->child1(), SpecCell);
-        break;
-    }
-
     case StoreBarrier: {
         filter(node->child1(), SpecCell);
         break;
index cb54e0b..a2b6546 100644 (file)
@@ -658,7 +658,6 @@ void clobberize(Graph& graph, Node* node, ReadFunctor& read, WriteFunctor& write
         return;
 
     case StoreBarrier:
-    case ConditionalStoreBarrier:
     case StoreBarrierWithNullCheck:
         read(BarrierState);
         write(BarrierState);
index 44d4136..c65720b 100644 (file)
@@ -253,14 +253,6 @@ private:
                 break;
             }
 
-            case ConditionalStoreBarrier: {
-                if (!m_interpreter.needsTypeCheck(node->child2().node(), ~SpecCell)) {
-                    node->convertToPhantom();
-                    eliminated = true;
-                }
-                break;
-            }
-
             case ToPrimitive: {
                 if (m_state.forNode(node->child1()).m_type & ~(SpecFullNumber | SpecBoolean | SpecString))
                     break;
@@ -471,9 +463,8 @@ private:
 
         node->convertToPutByOffset(m_graph.m_storageAccessData.size(), propertyStorage);
         m_insertionSet.insertNode(
-            indexInBlock, SpecNone, ConditionalStoreBarrier, origin, 
-            Edge(node->child2().node(), KnownCellUse),
-            Edge(node->child3().node(), UntypedUse));
+            indexInBlock, SpecNone, StoreBarrier, origin, 
+            Edge(node->child2().node(), KnownCellUse));
 
         StorageAccessData storageAccessData;
         storageAccessData.offset = variant.offset();
index 689b7ac..3c67923 100644 (file)
@@ -578,7 +578,7 @@ private:
             case Array::Arguments:
                 fixEdge<KnownCellUse>(child1);
                 fixEdge<Int32Use>(child2);
-                insertStoreBarrier(m_indexInBlock, child1, child3);
+                insertStoreBarrier(m_indexInBlock, child1);
                 break;
             default:
                 fixEdge<KnownCellUse>(child1);
@@ -616,7 +616,7 @@ private:
                 break;
             case Array::Contiguous:
             case Array::ArrayStorage:
-                insertStoreBarrier(m_indexInBlock, node->child1(), node->child2());
+                insertStoreBarrier(m_indexInBlock, node->child1());
                 break;
             default:
                 break;
@@ -809,7 +809,7 @@ private:
 
         case PutClosureVar: {
             fixEdge<KnownCellUse>(node->child1());
-            insertStoreBarrier(m_indexInBlock, node->child1(), node->child3());
+            insertStoreBarrier(m_indexInBlock, node->child1());
             break;
         }
 
@@ -853,7 +853,7 @@ private:
         case PutByIdFlush:
         case PutByIdDirect: {
             fixEdge<CellUse>(node->child1());
-            insertStoreBarrier(m_indexInBlock, node->child1(), node->child2());
+            insertStoreBarrier(m_indexInBlock, node->child1());
             break;
         }
 
@@ -892,13 +892,13 @@ private:
             if (!node->child1()->hasStorageResult())
                 fixEdge<KnownCellUse>(node->child1());
             fixEdge<KnownCellUse>(node->child2());
-            insertStoreBarrier(m_indexInBlock, node->child2(), node->child3());
+            insertStoreBarrier(m_indexInBlock, node->child2());
             break;
         }
             
         case MultiPutByOffset: {
             fixEdge<CellUse>(node->child1());
-            insertStoreBarrier(m_indexInBlock, node->child1(), node->child2());
+            insertStoreBarrier(m_indexInBlock, node->child1());
             break;
         }
             
@@ -966,8 +966,8 @@ private:
                 m_indexInBlock, SpecNone, WeakJSConstant, node->origin, 
                 OpInfo(m_graph.globalObjectFor(node->origin.semantic)));
             Node* barrierNode = m_graph.addNode(
-                SpecNone, ConditionalStoreBarrier, m_currentNode->origin, 
-                Edge(globalObjectNode, KnownCellUse), Edge(node->child1().node(), UntypedUse));
+                SpecNone, StoreBarrier, m_currentNode->origin, 
+                Edge(globalObjectNode, KnownCellUse));
             m_insertionSet.insert(m_indexInBlock, barrierNode);
             break;
         }
@@ -1041,7 +1041,6 @@ private:
         case ExtractOSREntryLocal:
         case LoopHint:
         case StoreBarrier:
-        case ConditionalStoreBarrier:
         case StoreBarrierWithNullCheck:
         case FunctionReentryWatchpoint:
         case TypedArrayWatchpoint:
@@ -1618,15 +1617,9 @@ private:
         edge.setUseKind(useKind);
     }
     
-    void insertStoreBarrier(unsigned indexInBlock, Edge child1, Edge child2 = Edge())
+    void insertStoreBarrier(unsigned indexInBlock, Edge child1)
     {
-        Node* barrierNode;
-        if (!child2)
-            barrierNode = m_graph.addNode(SpecNone, StoreBarrier, m_currentNode->origin, Edge(child1.node(), child1.useKind()));
-        else {
-            barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->origin, 
-                Edge(child1.node(), child1.useKind()), Edge(child2.node(), child2.useKind()));
-        }
+        Node* barrierNode = m_graph.addNode(SpecNone, StoreBarrier, m_currentNode->origin, child1);
         m_insertionSet.insert(indexInBlock, barrierNode);
     }
 
index 9cd343e..2c59c38 100644 (file)
@@ -640,7 +640,6 @@ struct Node {
     {
         switch (op()) {
         case StoreBarrier:
-        case ConditionalStoreBarrier:
         case StoreBarrierWithNullCheck:
             return true;
         default:
index 884c5e2..70cd27f 100644 (file)
@@ -296,7 +296,6 @@ namespace JSC { namespace DFG {
     macro(CheckWatchdogTimer, NodeMustGenerate) \
     /* Write barriers ! */\
     macro(StoreBarrier, NodeMustGenerate) \
-    macro(ConditionalStoreBarrier, NodeMustGenerate) \
     macro(StoreBarrierWithNullCheck, NodeMustGenerate) \
 
 // This enum generates a monotonically increasing id for all Node types,
index 76bb124..4e44924 100644 (file)
@@ -541,7 +541,6 @@ private:
 #ifndef NDEBUG
         // These get ignored because they don't return anything.
         case StoreBarrier:
-        case ConditionalStoreBarrier:
         case StoreBarrierWithNullCheck:
         case PutByValDirect:
         case PutByVal:
index 55e4282..23c4ac2 100644 (file)
@@ -243,7 +243,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node)
     case Int52ToDouble:
     case Int52ToValue:
     case StoreBarrier:
-    case ConditionalStoreBarrier:
     case StoreBarrierWithNullCheck:
     case InvalidationPoint:
     case NotifyWrite:
index f949e7d..8b87c1c 100644 (file)
@@ -5349,11 +5349,6 @@ void SpeculativeJIT::linkBranches()
 void SpeculativeJIT::compileStoreBarrier(Node* node)
 {
     switch (node->op()) {
-    case ConditionalStoreBarrier: {
-        compileBaseValueStoreBarrier(node->child1(), node->child2());
-        break;
-    }
-
     case StoreBarrier: {
         SpeculateCellOperand base(this, node->child1());
         GPRTemporary scratch1(this);
index 7cdfe1d..79adedc 100644 (file)
@@ -4647,7 +4647,6 @@ void SpeculativeJIT::compile(Node* node)
         break;
 
     case StoreBarrier:
-    case ConditionalStoreBarrier:
     case StoreBarrierWithNullCheck: {
         compileStoreBarrier(node);
         break;
index 9045f65..4187b91 100644 (file)
@@ -4963,7 +4963,6 @@ void SpeculativeJIT::compile(Node* node)
         break;
 
     case StoreBarrier:
-    case ConditionalStoreBarrier:
     case StoreBarrierWithNullCheck: {
         compileStoreBarrier(node);
         break;
index ff89cf5..dc23f3d 100644 (file)
@@ -115,7 +115,6 @@ inline CapabilityLevel canCompile(Node* node)
     case VariableWatchpoint:
     case NotifyWrite:
     case StoreBarrier:
-    case ConditionalStoreBarrier:
     case StoreBarrierWithNullCheck:
     case Call:
     case Construct:
index 1f5e847..d83bd69 100644 (file)
@@ -581,9 +581,6 @@ private:
         case StoreBarrier:
             compileStoreBarrier();
             break;
-        case ConditionalStoreBarrier:
-            compileConditionalStoreBarrier();
-            break;
         case StoreBarrierWithNullCheck:
             compileStoreBarrierWithNullCheck();
             break;
@@ -717,13 +714,6 @@ private:
         emitStoreBarrier(lowCell(m_node->child1()));
     }
 
-    void compileConditionalStoreBarrier()
-    {
-        LValue base = lowCell(m_node->child1());
-        LValue value = lowJSValue(m_node->child2());
-        emitStoreBarrier(base, value, m_node->child2());
-    }
-
     void compileStoreBarrierWithNullCheck()
     {
 #if ENABLE(GGC)
@@ -5373,29 +5363,6 @@ private:
         return m_out.load8(base, m_heaps.JSCell_gcData);
     }
 
-    void emitStoreBarrier(LValue base, LValue value, Edge valueEdge)
-    {
-#if ENABLE(GGC)
-        LBasicBlock continuation = FTL_NEW_BLOCK(m_out, ("Store barrier continuation"));
-        LBasicBlock isCell = FTL_NEW_BLOCK(m_out, ("Store barrier is cell block"));
-
-        if (m_state.forNode(valueEdge.node()).couldBeType(SpecCell))
-            m_out.branch(isNotCell(value), unsure(continuation), unsure(isCell));
-        else
-            m_out.jump(isCell);
-
-        LBasicBlock lastNext = m_out.appendTo(isCell, continuation);
-        emitStoreBarrier(base);
-        m_out.jump(continuation);
-
-        m_out.appendTo(continuation, lastNext);
-#else
-        UNUSED_PARAM(base);
-        UNUSED_PARAM(value);
-        UNUSED_PARAM(valueEdge);
-#endif
-    }
-
     void emitStoreBarrier(LValue base)
     {
 #if ENABLE(GGC)
index fdd2638..6c38a0d 100644 (file)
@@ -773,65 +773,6 @@ static V_JITOperation_ESsiJJI appropriateListBuildingPutByIdFunction(const PutPr
     return operationPutByIdNonStrictBuildList;
 }
 
-#if ENABLE(GGC)
-static MacroAssembler::Call storeToWriteBarrierBuffer(CCallHelpers& jit, GPRReg cell, GPRReg scratch1, GPRReg scratch2, ScratchRegisterAllocator& allocator)
-{
-    ASSERT(scratch1 != scratch2);
-    WriteBarrierBuffer* writeBarrierBuffer = &jit.vm()->heap.writeBarrierBuffer();
-    jit.move(MacroAssembler::TrustedImmPtr(writeBarrierBuffer), scratch1);
-    jit.load32(MacroAssembler::Address(scratch1, WriteBarrierBuffer::currentIndexOffset()), scratch2);
-    MacroAssembler::Jump needToFlush = jit.branch32(MacroAssembler::AboveOrEqual, scratch2, MacroAssembler::Address(scratch1, WriteBarrierBuffer::capacityOffset()));
-
-    jit.add32(MacroAssembler::TrustedImm32(1), scratch2);
-    jit.store32(scratch2, MacroAssembler::Address(scratch1, WriteBarrierBuffer::currentIndexOffset()));
-
-    jit.loadPtr(MacroAssembler::Address(scratch1, WriteBarrierBuffer::bufferOffset()), scratch1);
-    // We use an offset of -sizeof(void*) because we already added 1 to scratch2.
-    jit.storePtr(cell, MacroAssembler::BaseIndex(scratch1, scratch2, MacroAssembler::ScalePtr, static_cast<int32_t>(-sizeof(void*))));
-
-    MacroAssembler::Jump done = jit.jump();
-    needToFlush.link(&jit);
-
-    ScratchBuffer* scratchBuffer = jit.vm()->scratchBufferForSize(allocator.desiredScratchBufferSize());
-    allocator.preserveUsedRegistersToScratchBuffer(jit, scratchBuffer, scratch1);
-
-    unsigned bytesFromBase = allocator.numberOfReusedRegisters() * sizeof(void*);
-    unsigned bytesToSubtract = 0;
-#if CPU(X86)
-    bytesToSubtract += 2 * sizeof(void*);
-    bytesFromBase += bytesToSubtract;
-#endif
-    unsigned currentAlignment = bytesFromBase % stackAlignmentBytes();
-    bytesToSubtract += currentAlignment;
-
-    if (bytesToSubtract)
-        jit.subPtr(MacroAssembler::TrustedImm32(bytesToSubtract), MacroAssembler::stackPointerRegister); 
-
-    jit.setupArgumentsWithExecState(cell);
-    MacroAssembler::Call call = jit.call();
-
-    if (bytesToSubtract)
-        jit.addPtr(MacroAssembler::TrustedImm32(bytesToSubtract), MacroAssembler::stackPointerRegister);
-    allocator.restoreUsedRegistersFromScratchBuffer(jit, scratchBuffer, scratch1);
-
-    done.link(&jit);
-
-    return call;
-}
-
-static MacroAssembler::Call writeBarrier(CCallHelpers& jit, GPRReg owner, GPRReg scratch1, GPRReg scratch2, ScratchRegisterAllocator& allocator)
-{
-    ASSERT(owner != scratch1);
-    ASSERT(owner != scratch2);
-
-    MacroAssembler::Jump ownerNotMarkedOrAlreadyRemembered = jit.checkMarkByte(owner);
-    MacroAssembler::Call call = storeToWriteBarrierBuffer(jit, owner, scratch1, scratch2, allocator);
-    ownerNotMarkedOrAlreadyRemembered.link(&jit);
-
-    return call;
-}
-#endif // ENABLE(GGC)
-
 static void emitPutReplaceStub(
     ExecState* exec,
     JSValue,
@@ -1051,10 +992,6 @@ static void emitPutTransitionStub(
     }
 #endif
     
-#if ENABLE(GGC)
-    MacroAssembler::Call writeBarrierOperation = writeBarrier(stubJit, baseGPR, scratchGPR1, scratchGPR2, allocator);
-#endif
-    
     MacroAssembler::Jump success;
     MacroAssembler::Jump failure;
             
@@ -1088,9 +1025,6 @@ static void emitPutTransitionStub(
     }
     
     LinkBuffer patchBuffer(*vm, &stubJit, exec->codeBlock());
-#if ENABLE(GGC)
-    patchBuffer.link(writeBarrierOperation, operationFlushWriteBarrierBuffer);
-#endif
     patchBuffer.link(success, stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToDone));
     if (allocator.didReuseRegisters())
         patchBuffer.link(failure, failureLabel);