Changeset 129089 in webkit


Ignore:
Timestamp:
Sep 19, 2012 9:44:43 PM (12 years ago)
Author:
ggaren@apple.com
Message:

OSR exit sometimes neglects to create the arguments object
​https://bugs.webkit.org/show_bug.cgi?id=97162

Reviewed by Filip Pizlo.

../JavaScriptCore:

No performance change.

I don't know of any case where this is a real problem in TOT, but it
will become a problem if we start compiling eval, with, or catch, and/or
sometimes stop doing arguments optimizations in the bytecode.

  • dfg/DFGArgumentsSimplificationPhase.cpp:

(JSC::DFG::ArgumentsSimplificationPhase::run): Account for a
CreateArguments that has transformed into PhantomArguments. We used to
clear our reference to the CreateArguments node, but now we hold onto it,
so we need to account for it transforming.

Don't replace a SetLocal(CreateArguments) with a SetLocal(JSValue())
because that doesn't leave enough information behind for OSR exit to do
the right thing. Instead, maintain our reference to CreateArguments, and
rely on CreateArguments transforming into PhantomArguments after
optimization. SetLocal(PhantomArguments) is efficient, and it's a marker
for OSR exit to create the arguments object.

Don't ASSERT that all PhantomArguments are unreferenced because we now
leave them in the graph as SetLocal(PhantomArguments), and that's harmless.

  • dfg/DFGArgumentsSimplificationPhase.h:

(NullableHashTraits):
(JSC::DFG::NullableHashTraits::emptyValue): Export our special hash table
for inline call frames so the OSR exit compiler can use it.

  • dfg/DFGOSRExitCompiler32_64.cpp:

(JSC::DFG::OSRExitCompiler::compileExit):

  • dfg/DFGOSRExitCompiler64.cpp:

(JSC::DFG::OSRExitCompiler::compileExit): Don't load the 'arguments'
register to decide if we need to create the arguments object. Optimization
may have eliminated the initializing store to this register, in which
case we'll load garbage. Instead, use the global knowledge that all call
frames that optimized out 'arguments' now need to create it, and use a hash
table to make sure we do so only once per call frame.

  • dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::compile): SetLocal(PhantomArguments) is unique
because we haven't just changed a value's format or elided a load or store;
instead, we've replaced an object with JSValue(). We could try to account
for this in a general way, but for now it's a special-case optimization,
so we give it a specific OSR hint instead.

../WTF:

  • wtf/HashTraits.h:

(NullableHashTraits):
(WTF::NullableHashTraits::emptyValue):
(WTF):

Location:
trunk/Source
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r129065 r129089  
     12012-09-19  Geoffrey Garen  <ggaren@apple.com>
     2
     3        OSR exit sometimes neglects to create the arguments object
     4        https://bugs.webkit.org/show_bug.cgi?id=97162
     5
     6        Reviewed by Filip Pizlo.
     7
     8        No performance change.
     9
     10        I don't know of any case where this is a real problem in TOT, but it
     11        will become a problem if we start compiling eval, with, or catch, and/or
     12        sometimes stop doing arguments optimizations in the bytecode.
     13
     14        * dfg/DFGArgumentsSimplificationPhase.cpp:
     15        (JSC::DFG::ArgumentsSimplificationPhase::run): Account for a
     16        CreateArguments that has transformed into PhantomArguments. We used to
     17        clear our reference to the CreateArguments node, but now we hold onto it,
     18        so we need to account for it transforming.
     19
     20        Don't replace a SetLocal(CreateArguments) with a SetLocal(JSValue())
     21        because that doesn't leave enough information behind for OSR exit to do
     22        the right thing. Instead, maintain our reference to CreateArguments, and
     23        rely on CreateArguments transforming into PhantomArguments after
     24        optimization. SetLocal(PhantomArguments) is efficient, and it's a marker
     25        for OSR exit to create the arguments object.
     26
     27        Don't ASSERT that all PhantomArguments are unreferenced because we now
     28        leave them in the graph as SetLocal(PhantomArguments), and that's harmless.
     29
     30        * dfg/DFGArgumentsSimplificationPhase.h:
     31        (NullableHashTraits):
     32        (JSC::DFG::NullableHashTraits::emptyValue): Export our special hash table
     33        for inline call frames so the OSR exit compiler can use it.
     34
     35        * dfg/DFGOSRExitCompiler32_64.cpp:
     36        (JSC::DFG::OSRExitCompiler::compileExit):
     37        * dfg/DFGOSRExitCompiler64.cpp:
     38        (JSC::DFG::OSRExitCompiler::compileExit): Don't load the 'arguments'
     39        register to decide if we need to create the arguments object. Optimization
     40        may have eliminated the initializing store to this register, in which
     41        case we'll load garbage. Instead, use the global knowledge that all call
     42        frames that optimized out 'arguments' now need to create it, and use a hash
     43        table to make sure we do so only once per call frame.
     44
     45        * dfg/DFGSpeculativeJIT64.cpp:
     46        (JSC::DFG::SpeculativeJIT::compile): SetLocal(PhantomArguments) is unique
     47        because we haven't just changed a value's format or elided a load or store;
     48        instead, we've replaced an object with JSValue(). We could try to account
     49        for this in a general way, but for now it's a special-case optimization,
     50        so we give it a specific OSR hint instead.
     51
    1522012-09-19  Filip Pizlo  <fpizlo@apple.com>
    253

  • trunk/Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp

    r128544 r129089  
    4242namespace {
    4343
    44 template<typename T>
    45 struct NullableHashTraits : public HashTraits<T> {
    46     static const bool emptyValueIsZero = false;
    47     static T emptyValue() { return reinterpret_cast<T>(1); }
    48 };
    49 
    5044struct ArgumentsAliasingData {
    5145    InlineCallFrame* callContext;
    … …  
    182176                    int argumentsRegister =
    183177                        m_graph.uncheckedArgumentsRegisterFor(node.codeOrigin);
    184                     if (source.op() != CreateArguments) {
     178                    if (source.op() != CreateArguments && source.op() != PhantomArguments) {
    185179                        // Make sure that the source of the SetLocal knows that if it's
    186180                        // a variable that we think is aliased to the arguments, then it
    … …  
    436430                   
    437431                    if (m_graph.argumentsRegisterFor(node.codeOrigin) == variableAccessData->local()
    438                            || unmodifiedArgumentsRegister(m_graph.argumentsRegisterFor(node.codeOrigin)) == variableAccessData->local()) {
    439                         // The child of this store should really be the empty value.
    440                         Node emptyJSValue(JSConstant, node.codeOrigin, OpInfo(codeBlock()->addOrFindConstant(JSValue())));
    441                         emptyJSValue.ref();
    442                         NodeIndex emptyJSValueIndex = m_graph.size();
    443                         m_graph.deref(node.child1());
    444                         node.children.child1() = Edge(emptyJSValueIndex);
    445                         m_graph.append(emptyJSValue);
    446                         insertionSet.append(indexInBlock, emptyJSValueIndex);
    447                         changed = true;
    448                         break;
    449                     }
     432                           || unmodifiedArgumentsRegister(m_graph.argumentsRegisterFor(node.codeOrigin)) == variableAccessData->local())
     433                        break;
     434
    450435                    ASSERT(!variableAccessData->isCaptured());
    451436                   
    … …  
    662647        }
    663648       
    664         if (changed) {
     649        if (changed)
    665650            m_graph.collectGarbage();
    666            
    667             // Verify that PhantomArguments nodes are not shouldGenerate().
    668 #if !ASSERT_DISABLED
    669             for (BlockIndex blockIndex = 0; blockIndex < m_graph.m_blocks.size(); ++blockIndex) {
    670                 BasicBlock* block = m_graph.m_blocks[blockIndex].get();
    671                 if (!block)
    672                     continue;
    673                 for (unsigned indexInBlock = 0; indexInBlock < block->size(); ++indexInBlock) {
    674                     NodeIndex nodeIndex = block->at(indexInBlock);
    675                     Node& node = m_graph[nodeIndex];
    676                     if (node.op() != PhantomArguments)
    677                         continue;
    678                     ASSERT(!node.shouldGenerate());
    679                 }
    680             }
    681 #endif
    682         }
    683651       
    684652        return changed;

  • trunk/Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp

    r121717 r129089  
    613613   
    614614    if (haveArguments) {
     615        HashSet<InlineCallFrame*, DefaultHash<InlineCallFrame*>::Hash,
     616            NullableHashTraits<InlineCallFrame*> > didCreateArgumentsObject;
     617
    615618        for (size_t index = 0; index < operands.size(); ++index) {
    616619            const ValueRecovery& recovery = operands[index];
    … …  
    628631                }
    629632            }
     633
    630634            int argumentsRegister = m_jit.argumentsRegisterFor(inlineCallFrame);
    631            
     635            if (didCreateArgumentsObject.add(inlineCallFrame).isNewEntry) {
     636                // We know this call frame optimized out an arguments object that
     637                // the baseline JIT would have created. Do that creation now.
     638                if (inlineCallFrame) {
     639                    m_jit.setupArgumentsWithExecState(
     640                        AssemblyHelpers::TrustedImmPtr(inlineCallFrame));
     641                    m_jit.move(
     642                        AssemblyHelpers::TrustedImmPtr(
     643                            bitwise_cast<void*>(operationCreateInlinedArguments)),
     644                        GPRInfo::nonArgGPR0);
     645                } else {
     646                    m_jit.setupArgumentsExecState();
     647                    m_jit.move(
     648                        AssemblyHelpers::TrustedImmPtr(
     649                            bitwise_cast<void*>(operationCreateArguments)),
     650                        GPRInfo::nonArgGPR0);
     651                }
     652                m_jit.call(GPRInfo::nonArgGPR0);
     653                m_jit.store32(
     654                    AssemblyHelpers::TrustedImm32(JSValue::CellTag),
     655                    AssemblyHelpers::tagFor(argumentsRegister));
     656                m_jit.store32(
     657                    GPRInfo::returnValueGPR,
     658                    AssemblyHelpers::payloadFor(argumentsRegister));
     659                m_jit.store32(
     660                    AssemblyHelpers::TrustedImm32(JSValue::CellTag),
     661                    AssemblyHelpers::tagFor(unmodifiedArgumentsRegister(argumentsRegister)));
     662                m_jit.store32(
     663                    GPRInfo::returnValueGPR,
     664                    AssemblyHelpers::payloadFor(unmodifiedArgumentsRegister(argumentsRegister)));
     665                m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
     666            }
     667
    632668            m_jit.load32(AssemblyHelpers::payloadFor(argumentsRegister), GPRInfo::regT0);
    633             AssemblyHelpers::Jump haveArguments = m_jit.branch32(
    634                 AssemblyHelpers::NotEqual,
    635                 AssemblyHelpers::tagFor(argumentsRegister),
    636                 AssemblyHelpers::TrustedImm32(JSValue::EmptyValueTag));
    637            
    638             if (inlineCallFrame) {
    639                 m_jit.setupArgumentsWithExecState(
    640                     AssemblyHelpers::TrustedImmPtr(inlineCallFrame));
    641                 m_jit.move(
    642                     AssemblyHelpers::TrustedImmPtr(
    643                         bitwise_cast<void*>(operationCreateInlinedArguments)),
    644                     GPRInfo::nonArgGPR0);
    645             } else {
    646                 m_jit.setupArgumentsExecState();
    647                 m_jit.move(
    648                     AssemblyHelpers::TrustedImmPtr(
    649                         bitwise_cast<void*>(operationCreateArguments)),
    650                     GPRInfo::nonArgGPR0);
    651             }
    652             m_jit.call(GPRInfo::nonArgGPR0);
    653             m_jit.store32(
    654                 AssemblyHelpers::TrustedImm32(JSValue::CellTag),
    655                 AssemblyHelpers::tagFor(argumentsRegister));
    656             m_jit.store32(
    657                 GPRInfo::returnValueGPR,
    658                 AssemblyHelpers::payloadFor(argumentsRegister));
    659             m_jit.store32(
    660                 AssemblyHelpers::TrustedImm32(JSValue::CellTag),
    661                 AssemblyHelpers::tagFor(unmodifiedArgumentsRegister(argumentsRegister)));
    662             m_jit.store32(
    663                 GPRInfo::returnValueGPR,
    664                 AssemblyHelpers::payloadFor(unmodifiedArgumentsRegister(argumentsRegister)));
    665             m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
    666            
    667             haveArguments.link(&m_jit);
    668669            m_jit.store32(
    669670                AssemblyHelpers::TrustedImm32(JSValue::CellTag),

  • trunk/Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp

    r121717 r129089  
    588588   
    589589    if (haveArguments) {
     590        HashSet<InlineCallFrame*, DefaultHash<InlineCallFrame*>::Hash,
     591            NullableHashTraits<InlineCallFrame*> > didCreateArgumentsObject;
     592
    590593        for (size_t index = 0; index < operands.size(); ++index) {
    591594            const ValueRecovery& recovery = operands[index];
    … …  
    603606                }
    604607            }
     608
    605609            int argumentsRegister = m_jit.argumentsRegisterFor(inlineCallFrame);
    606            
     610            if (didCreateArgumentsObject.add(inlineCallFrame).isNewEntry) {
     611                // We know this call frame optimized out an arguments object that
     612                // the baseline JIT would have created. Do that creation now.
     613                if (inlineCallFrame) {
     614                    m_jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->stackOffset * sizeof(EncodedJSValue)), GPRInfo::callFrameRegister, GPRInfo::regT0);
     615                    m_jit.setupArguments(GPRInfo::regT0);
     616                } else
     617                    m_jit.setupArgumentsExecState();
     618                m_jit.move(
     619                    AssemblyHelpers::TrustedImmPtr(
     620                        bitwise_cast<void*>(operationCreateArguments)),
     621                    GPRInfo::nonArgGPR0);
     622                m_jit.call(GPRInfo::nonArgGPR0);
     623                m_jit.storePtr(GPRInfo::returnValueGPR, AssemblyHelpers::addressFor(argumentsRegister));
     624                m_jit.storePtr(
     625                    GPRInfo::returnValueGPR,
     626                    AssemblyHelpers::addressFor(unmodifiedArgumentsRegister(argumentsRegister)));
     627                m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
     628            }
     629
    607630            m_jit.loadPtr(AssemblyHelpers::addressFor(argumentsRegister), GPRInfo::regT0);
    608             AssemblyHelpers::Jump haveArguments = m_jit.branchTestPtr(
    609                 AssemblyHelpers::NonZero, GPRInfo::regT0);
    610            
    611             if (inlineCallFrame) {
    612                 m_jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->stackOffset * sizeof(EncodedJSValue)), GPRInfo::callFrameRegister, GPRInfo::regT0);
    613                 m_jit.setupArguments(GPRInfo::regT0);
    614             } else
    615                 m_jit.setupArgumentsExecState();
    616             m_jit.move(
    617                 AssemblyHelpers::TrustedImmPtr(
    618                     bitwise_cast<void*>(operationCreateArguments)),
    619                 GPRInfo::nonArgGPR0);
    620             m_jit.call(GPRInfo::nonArgGPR0);
    621             m_jit.storePtr(GPRInfo::returnValueGPR, AssemblyHelpers::addressFor(argumentsRegister));
    622             m_jit.storePtr(
    623                 GPRInfo::returnValueGPR,
    624                 AssemblyHelpers::addressFor(unmodifiedArgumentsRegister(argumentsRegister)));
    625             m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
    626            
    627             haveArguments.link(&m_jit);
    628631            m_jit.storePtr(GPRInfo::regT0, AssemblyHelpers::addressFor(operand));
    629632        }

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp

    r129045 r129089  
    20562056
    20572057    case PhantomArguments:
    2058         // This should never be must-generate.
    2059         ASSERT_NOT_REACHED();
    2060         // But as a release-mode fall-back make it the empty value.
    20612058        initConstantInfo(m_compileIndex);
    20622059        break;
    … …  
    22452242        noResult(m_compileIndex);
    22462243        recordSetLocal(node.local(), ValueSource(ValueInRegisterFile));
     2244
     2245        // If we're storing an arguments object that has been optimized away,
     2246        // our variable event stream for OSR exit now reflects the optimized
     2247        // value (JSValue()). On the slow path, we want an arguments object
     2248        // instead. We add an additional move hint to show OSR exit that it
     2249        // needs to reconstruct the arguments object.
     2250        if (at(node.child1()).op() == PhantomArguments)
     2251            compileMovHint(node);
     2252
    22472253        break;
    22482254    }

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

    r129045 r129089  
    21202120
    21212121    case PhantomArguments:
    2122         // This should never be must-generate.
    2123         ASSERT_NOT_REACHED();
    2124         // But as a release-mode fall-back make it the empty value.
    21252122        initConstantInfo(m_compileIndex);
    21262123        break;
    … …  
    22812278
    22822279        recordSetLocal(node.local(), ValueSource(ValueInRegisterFile));
     2280
     2281        // If we're storing an arguments object that has been optimized away,
     2282        // our variable event stream for OSR exit now reflects the optimized
     2283        // value (JSValue()). On the slow path, we want an arguments object
     2284        // instead. We add an additional move hint to show OSR exit that it
     2285        // needs to reconstruct the arguments object.
     2286        if (at(node.child1()).op() == PhantomArguments)
     2287            compileMovHint(node);
     2288
    22832289        break;
    22842290    }

  • trunk/Source/WTF/ChangeLog

    r128908 r129089  
     12012-09-19  Geoffrey Garen  <ggaren@apple.com>
     2
     3        OSR exit sometimes neglects to create the arguments object
     4        https://bugs.webkit.org/show_bug.cgi?id=97162
     5
     6        Reviewed by Filip Pizlo.
     7
     8        * wtf/HashTraits.h:
     9        (NullableHashTraits):
     10        (WTF::NullableHashTraits::emptyValue):
     11        (WTF):
     12
    1132012-09-18  Glenn Adams  <glenn@skynav.com>
    214

  • trunk/Source/WTF/wtf/HashTraits.h

    r126926 r129089  
    232232    struct HashTraits<KeyValuePair<Key, Value> > : public KeyValuePairHashTraits<HashTraits<Key>, HashTraits<Value> > { };
    233233
     234    template<typename T>
     235    struct NullableHashTraits : public HashTraits<T> {
     236        static const bool emptyValueIsZero = false;
     237        static T emptyValue() { return reinterpret_cast<T>(1); }
     238    };
     239
    234240} // namespace WTF
    235241
    236242using WTF::HashTraits;
    237243using WTF::PairHashTraits;
     244using WTF::NullableHashTraits;
    238245
    239246#endif // WTF_HashTraits_h
Note: See TracChangeset for help on using the changeset viewer.