Changeset 123169 in webkit

Timestamp:

Jul 19, 2012 8:50:02 PM (12 years ago)

Author:

fpizlo@apple.com

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• assembler/MacroAssemblerX86_64.h (modified) (1 diff)
• assembler/X86Assembler.h (modified) (1 diff)
• dfg/DFGAbstractState.cpp (modified) (3 diffs)
• dfg/DFGDriver.cpp (modified) (1 diff)
• dfg/DFGGraph.h (modified) (1 diff)
• dfg/DFGSpeculativeJIT.cpp (modified) (2 diffs)
• dfg/DFGSpeculativeJIT32_64.cpp (modified) (3 diffs)
• dfg/DFGSpeculativeJIT64.cpp (modified) (2 diffs)
• dfg/DFGValueSource.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2012-07-18  Filip Pizlo  <fpizlo@apple.com>
+
+        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  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h

@@ -477 +477 @@
     }
 
+    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))
     {

trunk/Source/JavaScriptCore/assembler/X86Assembler.h

@@ -998 +998 @@
     }
 
+    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)
     {

trunk/Source/JavaScriptCore/dfg/DFGAbstractState.cpp

@@ -129 +129 @@
         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();
@@ -273 +275 @@
         
     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);
@@ -291 +294 @@
             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);

trunk/Source/JavaScriptCore/dfg/DFGDriver.cpp

@@ -65 +65 @@
     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());

trunk/Source/JavaScriptCore/dfg/DFGGraph.h

@@ -442 +442 @@
     }
     
+    bool isCreatedThisArgument(int operand)
+    {
+        if (!operandIsArgument(operand))
+            return false;
+        if (operandToArgument(operand))
+            return false;
+        return m_codeBlock->specializationKind() == CodeForConstruct;
+    }
+    
     unsigned numSuccessors(BasicBlock* block)
     {

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -1242 +1242 @@
             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))
@@ -1308 +1309 @@
             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
     }

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp

@@ -1919 +1919 @@
             }
 
-            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());
@@ -2011 +2011 @@
         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());
@@ -2047 +2047 @@
                 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());

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

@@ -2053 +2053 @@
         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());
@@ -2083 +2083 @@
                 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());

trunk/Source/JavaScriptCore/dfg/DFGValueSource.h

@@ -131 +131 @@
         if (isInt32Speculation(prediction))
             return ValueSource(Int32InRegisterFile);
-        if (isArraySpeculation(prediction))
+        if (isArraySpeculation(prediction) || isCellSpeculation(prediction))
             return ValueSource(CellInRegisterFile);
         if (isBooleanSpeculation(prediction))