[webkit-changes] [123169] trunk/Source/JavaScriptCore

Thu, 19 Jul 2012 20:50:15 -0700

Title: [123169] trunk/Source/_javascript_Core
Revision
123169
Author
[email protected]
Date
2012-07-19 20:50:02 -0700 (Thu, 19 Jul 2012)

Log Message

DFG cell checks should be hoisted
https://bugs.webkit.org/show_bug.cgi?id=91717

Reviewed by Geoffrey Garen.

The DFG has always had the policy of hoisting array and integer checks to
the point of variable assignment. Eventually, we added doubles and booleans
to the mix. But cells should really be part of this as well, particularly
for 32-bit where accessing a known-type variable is dramatically cheaper
than accessing a variable whose types is only predicted but otherwise
unproven.
        
This appears to be a definite speed-up for V8 on 32-bit, a possible speed-up
for Kraken, and a possible slow-down for V8 on 64-bit (around 0.2% if at
all). Any slow-downs can, and should, be addressed by making the hoisting
logic cognizant of variables that are never used in a manner that requires
type checks, and by sinking argument checks to the point(s) of first use.
        
To make this work I had to change some OSR machinery, and special-case the
type predictions of the 'this' argument for constructors. OSR exit normally
assumes that arguments are boxed, which happens to be true because the
type prediction used for check hoisting is LUB'd with the type of the
argument that was passed in - so either the arguments are always stored to
with the full tag+payload, or if only the payload is stored then the tag
matches whatever the caller would have set. But not so with the 'this'
argument for constructors, which is not initialized by the caller. We
could make this more precise by having argument types for OSR be inferred
using similar machinery to other locals, but I figured that for this patch
I should use the surgical fix.

* assembler/MacroAssemblerX86_64.h:
(JSC::MacroAssemblerX86_64::branchTestPtr):
(MacroAssemblerX86_64):
* assembler/X86Assembler.h:
(JSC::X86Assembler::testq_rm):
(X86Assembler):
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::initialize):
(JSC::DFG::AbstractState::execute):
* dfg/DFGDriver.cpp:
(JSC::DFG::compile):
* dfg/DFGGraph.h:
(JSC::DFG::Graph::isCreatedThisArgument):
(Graph):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::checkArgumentTypes):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGValueSource.h:
(JSC::DFG::ValueSource::forSpeculation):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (123168 => 123169)


--- trunk/Source/_javascript_Core/ChangeLog	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-07-20 03:50:02 UTC (rev 123169)
@@ -1,3 +1,58 @@
+2012-07-18  Filip Pizlo  <[email protected]>
+
+        DFG cell checks should be hoisted
+        https://bugs.webkit.org/show_bug.cgi?id=91717
+
+        Reviewed by Geoffrey Garen.
+
+        The DFG has always had the policy of hoisting array and integer checks to
+        the point of variable assignment. Eventually, we added doubles and booleans
+        to the mix. But cells should really be part of this as well, particularly
+        for 32-bit where accessing a known-type variable is dramatically cheaper
+        than accessing a variable whose types is only predicted but otherwise
+        unproven.
+        
+        This appears to be a definite speed-up for V8 on 32-bit, a possible speed-up
+        for Kraken, and a possible slow-down for V8 on 64-bit (around 0.2% if at
+        all). Any slow-downs can, and should, be addressed by making the hoisting
+        logic cognizant of variables that are never used in a manner that requires
+        type checks, and by sinking argument checks to the point(s) of first use.
+        
+        To make this work I had to change some OSR machinery, and special-case the
+        type predictions of the 'this' argument for constructors. OSR exit normally
+        assumes that arguments are boxed, which happens to be true because the
+        type prediction used for check hoisting is LUB'd with the type of the
+        argument that was passed in - so either the arguments are always stored to
+        with the full tag+payload, or if only the payload is stored then the tag
+        matches whatever the caller would have set. But not so with the 'this'
+        argument for constructors, which is not initialized by the caller. We
+        could make this more precise by having argument types for OSR be inferred
+        using similar machinery to other locals, but I figured that for this patch
+        I should use the surgical fix.
+
+        * assembler/MacroAssemblerX86_64.h:
+        (JSC::MacroAssemblerX86_64::branchTestPtr):
+        (MacroAssemblerX86_64):
+        * assembler/X86Assembler.h:
+        (JSC::X86Assembler::testq_rm):
+        (X86Assembler):
+        * dfg/DFGAbstractState.cpp:
+        (JSC::DFG::AbstractState::initialize):
+        (JSC::DFG::AbstractState::execute):
+        * dfg/DFGDriver.cpp:
+        (JSC::DFG::compile):
+        * dfg/DFGGraph.h:
+        (JSC::DFG::Graph::isCreatedThisArgument):
+        (Graph):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::checkArgumentTypes):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGValueSource.h:
+        (JSC::DFG::ValueSource::forSpeculation):
+
 2012-07-19  Filip Pizlo  <[email protected]>
 
         Fast path of storage resize should be removed from property storage reallocation, since it is only useful for arrays

Modified: trunk/Source/_javascript_Core/assembler/MacroAssemblerX86_64.h (123168 => 123169)


--- trunk/Source/_javascript_Core/assembler/MacroAssemblerX86_64.h	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/assembler/MacroAssemblerX86_64.h	2012-07-20 03:50:02 UTC (rev 123169)
@@ -476,6 +476,12 @@
         return Jump(m_assembler.jCC(x86Condition(cond)));
     }
 
+    Jump branchTestPtr(ResultCondition cond, Address address, RegisterID reg)
+    {
+        m_assembler.testq_rm(reg, address.offset, address.base);
+        return Jump(m_assembler.jCC(x86Condition(cond)));
+    }
+
     Jump branchTestPtr(ResultCondition cond, BaseIndex address, TrustedImm32 mask = TrustedImm32(-1))
     {
         if (mask.m_value == -1)

Modified: trunk/Source/_javascript_Core/assembler/X86Assembler.h (123168 => 123169)


--- trunk/Source/_javascript_Core/assembler/X86Assembler.h	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/assembler/X86Assembler.h	2012-07-20 03:50:02 UTC (rev 123169)
@@ -997,6 +997,11 @@
         m_formatter.oneByteOp64(OP_TEST_EvGv, src, dst);
     }
 
+    void testq_rm(RegisterID src, int offset, RegisterID base)
+    {
+        m_formatter.oneByteOp64(OP_TEST_EvGv, src, base, offset);
+    }
+
     void testq_i32r(int imm, RegisterID dst)
     {
         m_formatter.oneByteOp64(OP_GROUP3_EvIz, GROUP3_OP_TEST, dst);

Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp	2012-07-20 03:50:02 UTC (rev 123169)
@@ -128,6 +128,8 @@
             root->valuesAtHead.argument(i).set(SpecFloat32Array);
         else if (isFloat64ArraySpeculation(prediction))
             root->valuesAtHead.argument(i).set(SpecFloat64Array);
+        else if (isCellSpeculation(prediction))
+            root->valuesAtHead.argument(i).set(SpecCell);
         else
             root->valuesAtHead.argument(i).makeTop();
         
@@ -272,7 +274,8 @@
     }
         
     case SetLocal: {
-        if (node.variableAccessData()->isCaptured()) {
+        if (node.variableAccessData()->isCaptured()
+            || m_graph.isCreatedThisArgument(node.local())) {
             m_variables.operand(node.local()) = forNode(node.child1());
             node.setCanExit(false);
             break;
@@ -290,6 +293,9 @@
         else if (isArraySpeculation(predictedType)) {
             node.setCanExit(!isArraySpeculation(forNode(node.child1()).m_type));
             forNode(node.child1()).filter(SpecArray);
+        } else if (isCellSpeculation(predictedType)) {
+            node.setCanExit(!isCellSpeculation(forNode(node.child1()).m_type));
+            forNode(node.child1()).filter(SpecCell);
         } else if (isBooleanSpeculation(predictedType))
             speculateBooleanUnary(node);
         else

Modified: trunk/Source/_javascript_Core/dfg/DFGDriver.cpp (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGDriver.cpp	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGDriver.cpp	2012-07-20 03:50:02 UTC (rev 123169)
@@ -64,7 +64,7 @@
 
     if (!Options::useDFGJIT())
         return false;
-
+    
 #if DFG_ENABLE(DEBUG_VERBOSE)
     dataLog("DFG compiling code block %p(%p) for executable %p, number of instructions = %u.\n", codeBlock, codeBlock->alternative(), codeBlock->ownerExecutable(), codeBlock->instructionCount());
 #endif

Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-07-20 03:50:02 UTC (rev 123169)
@@ -441,6 +441,15 @@
         return m_codeBlock->usesArguments();
     }
     
+    bool isCreatedThisArgument(int operand)
+    {
+        if (!operandIsArgument(operand))
+            return false;
+        if (operandToArgument(operand))
+            return false;
+        return m_codeBlock->specializationKind() == CodeForConstruct;
+    }
+    
     unsigned numSuccessors(BasicBlock* block)
     {
         return at(block->last()).numSuccessors();

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-07-20 03:50:02 UTC (rev 123169)
@@ -1241,7 +1241,8 @@
             m_jit.loadPtr(JITCompiler::addressFor(virtualRegister), temp.gpr());
             speculationCheck(BadType, valueSource, nodeIndex, m_jit.branchTestPtr(MacroAssembler::NonZero, temp.gpr(), GPRInfo::tagMaskRegister));
             speculationCheck(BadType, valueSource, nodeIndex, m_jit.branchPtr(MacroAssembler::NotEqual, MacroAssembler::Address(temp.gpr(), JSCell::classInfoOffset()), MacroAssembler::TrustedImmPtr(m_jit.globalData()->float64ArrayDescriptor().m_classInfo)));
-        }
+        } else if (isCellSpeculation(predictedType))
+            speculationCheck(BadType, valueSource, nodeIndex, m_jit.branchTestPtr(MacroAssembler::NonZero, JITCompiler::addressFor(virtualRegister), GPRInfo::tagMaskRegister));
 #else
         if (isInt32Speculation(predictedType))
             speculationCheck(BadType, valueSource, nodeIndex, m_jit.branch32(MacroAssembler::NotEqual, JITCompiler::tagFor(virtualRegister), TrustedImm32(JSValue::Int32Tag)));
@@ -1307,7 +1308,8 @@
             speculationCheck(BadType, valueSource, nodeIndex, m_jit.branch32(MacroAssembler::NotEqual, temp.gpr(), TrustedImm32(JSValue::CellTag)));
             m_jit.load32(JITCompiler::payloadFor(virtualRegister), temp.gpr());
             speculationCheck(BadType, valueSource, nodeIndex, m_jit.branchPtr(MacroAssembler::NotEqual, MacroAssembler::Address(temp.gpr(), JSCell::classInfoOffset()), MacroAssembler::TrustedImmPtr(m_jit.globalData()->float64ArrayDescriptor().m_classInfo)));
-        } 
+        }   else if (isCellSpeculation(predictedType))
+            speculationCheck(BadType, valueSource, nodeIndex, m_jit.branch32(MacroAssembler::NotEqual, JITCompiler::tagFor(virtualRegister), TrustedImm32(JSValue::CellTag)));
 #endif
     }
     m_isCheckingArgumentTypes = false;

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2012-07-20 03:50:02 UTC (rev 123169)
@@ -1918,7 +1918,7 @@
                 break;
             }
 
-            if (isArraySpeculation(value.m_type)) {
+            if (isArraySpeculation(value.m_type) || isCellSpeculation(value.m_type)) {
                 GPRTemporary result(this);
                 m_jit.load32(JITCompiler::payloadFor(node.local()), result.gpr());
 
@@ -2010,7 +2010,7 @@
         // OSR exit, would not be visible to the old JIT in any way.
         m_codeOriginForOSR = nextNode->codeOrigin;
         
-        if (!node.variableAccessData()->isCaptured()) {
+        if (!node.variableAccessData()->isCaptured() && !m_jit.graph().isCreatedThisArgument(node.local())) {
             if (node.variableAccessData()->shouldUseDoubleFormat()) {
                 SpeculateDoubleOperand value(this, node.child1());
                 m_jit.storeDouble(value.fpr(), JITCompiler::addressFor(node.local()));
@@ -2046,6 +2046,14 @@
                 recordSetLocal(node.local(), ValueSource(CellInRegisterFile));
                 break;
             }
+            if (isCellSpeculation(predictedType)) {
+                SpeculateCellOperand cell(this, node.child1());
+                GPRReg cellGPR = cell.gpr();
+                m_jit.storePtr(cellGPR, JITCompiler::payloadFor(node.local()));
+                noResult(m_compileIndex);
+                recordSetLocal(node.local(), ValueSource(CellInRegisterFile));
+                break;
+            }
             if (isBooleanSpeculation(predictedType)) {
                 SpeculateBooleanOperand value(this, node.child1());
                 m_jit.store32(value.gpr(), JITCompiler::payloadFor(node.local()));

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2012-07-20 03:50:02 UTC (rev 123169)
@@ -2052,7 +2052,7 @@
         // OSR exit, would not be visible to the old JIT in any way.
         m_codeOriginForOSR = nextNode->codeOrigin;
         
-        if (!node.variableAccessData()->isCaptured()) {
+        if (!node.variableAccessData()->isCaptured() && !m_jit.graph().isCreatedThisArgument(node.local())) {
             if (node.variableAccessData()->shouldUseDoubleFormat()) {
                 SpeculateDoubleOperand value(this, node.child1());
                 m_jit.storeDouble(value.fpr(), JITCompiler::addressFor(node.local()));
@@ -2082,6 +2082,14 @@
                 recordSetLocal(node.local(), ValueSource(CellInRegisterFile));
                 break;
             }
+            if (isCellSpeculation(predictedType)) {
+                SpeculateCellOperand cell(this, node.child1());
+                GPRReg cellGPR = cell.gpr();
+                m_jit.storePtr(cellGPR, JITCompiler::addressFor(node.local()));
+                noResult(m_compileIndex);
+                recordSetLocal(node.local(), ValueSource(CellInRegisterFile));
+                break;
+            }
             if (isBooleanSpeculation(predictedType)) {
                 SpeculateBooleanOperand boolean(this, node.child1());
                 m_jit.storePtr(boolean.gpr(), JITCompiler::addressFor(node.local()));

Modified: trunk/Source/_javascript_Core/dfg/DFGValueSource.h (123168 => 123169)


--- trunk/Source/_javascript_Core/dfg/DFGValueSource.h	2012-07-20 01:31:55 UTC (rev 123168)
+++ trunk/Source/_javascript_Core/dfg/DFGValueSource.h	2012-07-20 03:50:02 UTC (rev 123169)
@@ -130,7 +130,7 @@
     {
         if (isInt32Speculation(prediction))
             return ValueSource(Int32InRegisterFile);
-        if (isArraySpeculation(prediction))
+        if (isArraySpeculation(prediction) || isCellSpeculation(prediction))
             return ValueSource(CellInRegisterFile);
         if (isBooleanSpeculation(prediction))
             return ValueSource(BooleanInRegisterFile);

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to