diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index cf846b8144a06..e9c88e3202685 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,78 @@ +2013-09-14 Filip Pizlo + + It should be easy to add new nodes that do OSR forward rewiring in both DFG and FTL + https://bugs.webkit.org/show_bug.cgi?id=121371 + + Reviewed by Sam Weinig. + + Forward rewiring is a tricky part of OSR that handles the following: + + a: Something(...) + SetLocal(@a, locX) + b: Int32ToDouble(@a) + c: SomethingThatExits(@b) + + + Note that at @c, OSR will think that locX->@a, but @a will be dead. So it must be + smart enough to find @b, which contains an equivalent value. It must do this for + any identity functions we support. Currently we support four such functions. + + Currently the code for doing this is basically duplicated between the DFG and the + FTL. Also both versions of the code have some really weirdly written logic for + picking the "best" identity function to use. + + We should fix this by simply having a way to ask "is this node an identity + function, and if so, then how good is it?" Then both the DFG and FTL could use + this and have no hard-wired knowledge of those identity functions. + + While we're at it, this also changes some terminology because I found the use of + the word "needs" confusing. Note that this retains the somewhat confusing behavior + that we don't search all possible forward/backward uses. We only search one step + in each direction. This is because we only need to handle cases that FixupPhase + and the parser insert. All other code that tries to insert intermediate conversion + nodes should ensure to Phantom the original node. For example, the following + transformation is illegal: + + Before: + x: SomethingThatExits(@a) + + After: + w: Conversion(@a) + x: SomethingThatExits(@w) + + The correct form of that transformation is one of these: + + Correct #1: + + v: DoAllChecks(@a) // exit here + w: Conversion(@a) + x: Something(@w) // no exit + + Correct #2: + + w: Conversion(@a) + x: SomethingThatExits(@w) + y: Phantom(@a) + + Correct #3: + + w: Conversion(@a) + x: SomethingThatExits(@w, @a) + + Note that we use #3 for some heap accesses, but of course it requires that the + node you're using has an extra slot for a "dummy" use child. + + Broadly speaking though, such transformations should be relegated to something + below DFG IR, like LLVM IR. + + * dfg/DFGNodeType.h: + (JSC::DFG::forwardRewiringSelectionScore): + (JSC::DFG::needsOSRForwardRewiring): + * dfg/DFGVariableEventStream.cpp: + (JSC::DFG::VariableEventStream::reconstruct): + * ftl/FTLLowerDFGToLLVM.cpp: + (JSC::FTL::LowerDFGToLLVM::addExitArgumentForNode): + 2013-09-14 Filip Pizlo Rename IntegerBranch/IntegerCompare to Int32Branch/Int32Compare. diff --git a/Source/JavaScriptCore/dfg/DFGMinifiedNode.h b/Source/JavaScriptCore/dfg/DFGMinifiedNode.h index 93303ccd05eab..1a24e39191e37 100644 --- a/Source/JavaScriptCore/dfg/DFGMinifiedNode.h +++ b/Source/JavaScriptCore/dfg/DFGMinifiedNode.h @@ -50,7 +50,7 @@ inline bool belongsInMinifiedGraph(NodeType type) case PhantomArguments: return true; default: - ASSERT(!needsOSRBackwardRewiring(type) && !needsOSRForwardRewiring(type)); + ASSERT(!permitsOSRBackwardRewiring(type) && !permitsOSRForwardRewiring(type)); return false; } } diff --git a/Source/JavaScriptCore/dfg/DFGNodeType.h b/Source/JavaScriptCore/dfg/DFGNodeType.h index 02f4bb559a220..82d373fe13f88 100644 --- a/Source/JavaScriptCore/dfg/DFGNodeType.h +++ b/Source/JavaScriptCore/dfg/DFGNodeType.h @@ -305,24 +305,67 @@ inline NodeFlags defaultFlags(NodeType op) } } -inline bool needsOSRBackwardRewiring(NodeType op) +inline bool permitsOSRBackwardRewiring(NodeType op) { - return op == UInt32ToNumber; + switch (op) { + case Identity: + RELEASE_ASSERT_NOT_REACHED(); + return true; + case UInt32ToNumber: + // This is the only node where we do: + // + // b: UInt32ToNumber(@a) + // c: SetLocal(@b) + // + // and then also have some uses of @a without Phantom'ing @b. + return true; + default: + return false; + } } -inline bool needsOSRForwardRewiring(NodeType op) +// Returns the priority with which we should select the given node for forward +// rewiring. Higher is better. Zero means that the node is not useful for rewiring. +// By convention, we use 100 to mean that the node is totally equivalent to its +// input with no information loss. +inline unsigned forwardRewiringSelectionScore(NodeType op) { switch (op) { + case Identity: + // We shouldn't see these by the time we get to OSR even though it clearly + // is a perfect identity function. + RELEASE_ASSERT_NOT_REACHED(); + return 100; + + case DoubleAsInt32: + // This speculates that the incoming double is convertible to an int32. So + // its result is totally equivalent. + return 100; + case Int32ToDouble: - case ValueToInt32: + // This converts an int32 to a double, but that loses a bit of information. + // OTOH it's still an equivalent number. + return 75; + case UInt32ToNumber: - case DoubleAsInt32: - return true; + // It's completely fine to use this for OSR exit, since the uint32 isn't + // actually representable in bytecode. + return 100; + + case ValueToInt32: + // This loses information. Only use it if there are no better alternatives. + return 25; + default: - return false; + return 0; } } +inline bool permitsOSRForwardRewiring(NodeType op) +{ + return forwardRewiringSelectionScore(op) > 0; +} + } } // namespace JSC::DFG #endif // ENABLE(DFG_JIT) diff --git a/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp b/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp index ab4c4ebac4be3..391827feb8fd4 100644 --- a/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp +++ b/Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp @@ -193,28 +193,26 @@ void VariableEventStream::reconstruct( MinifiedGenerationInfo info = generationInfos.get(source.id()); if (info.format == DataFormatNone) { // Try to see if there is an alternate node that would contain the value we want. - // There are four possibilities: // - // Int32ToDouble: We can use this in place of the original node, but - // we'd rather not; so we use it only if it is the only remaining - // live version. + // Backward rewiring refers to: // - // ValueToInt32: If the only remaining live version of the value is - // ValueToInt32, then we can use it. + // a: Something(...) + // b: Id(@a) // some identity function + // c: SetLocal(@b) // - // UInt32ToNumber: If the only live version of the value is a UInt32ToNumber - // then the only remaining uses are ones that want a properly formed number - // rather than a UInt32 intermediate. + // Where we find @b being dead, but @a is still alive. // - // DoubleAsInt32: Same as UInt32ToNumber. + // Forward rewiring refers to: // - // The reverse of the above: This node could be a UInt32ToNumber, but its - // alternative is still alive. This means that the only remaining uses of - // the number would be fine with a UInt32 intermediate. + // a: Something(...) + // b: SetLocal(@a) + // c: Id(@a) // some identity function + // + // Where we find @a being dead, but @b is still alive. bool found = false; - if (node && needsOSRBackwardRewiring(node->op())) { + if (node && permitsOSRBackwardRewiring(node->op())) { MinifiedID id = node->child1(); if (tryToSetConstantRecovery(valueRecoveries[i], codeBlock, graph.at(id))) continue; @@ -224,10 +222,8 @@ void VariableEventStream::reconstruct( } if (!found) { - MinifiedID int32ToDoubleID; - MinifiedID valueToInt32ID; - MinifiedID uint32ToNumberID; - MinifiedID doubleAsInt32ID; + MinifiedID bestID; + unsigned bestScore = 0; HashMap::iterator iter = generationInfos.begin(); HashMap::iterator end = generationInfos.end(); @@ -242,37 +238,15 @@ void VariableEventStream::reconstruct( continue; if (iter->value.format == DataFormatNone) continue; - switch (node->op()) { - case Int32ToDouble: - int32ToDoubleID = id; - break; - case ValueToInt32: - valueToInt32ID = id; - break; - case UInt32ToNumber: - uint32ToNumberID = id; - break; - case DoubleAsInt32: - doubleAsInt32ID = id; - break; - default: - ASSERT(!needsOSRForwardRewiring(node->op())); - break; - } + unsigned myScore = forwardRewiringSelectionScore(node->op()); + if (myScore <= bestScore) + continue; + bestID = id; + bestScore = myScore; } - MinifiedID idToUse; - if (!!doubleAsInt32ID) - idToUse = doubleAsInt32ID; - else if (!!int32ToDoubleID) - idToUse = int32ToDoubleID; - else if (!!valueToInt32ID) - idToUse = valueToInt32ID; - else if (!!uint32ToNumberID) - idToUse = uint32ToNumberID; - - if (!!idToUse) { - info = generationInfos.get(idToUse); + if (!!bestID) { + info = generationInfos.get(bestID); ASSERT(info.format != DataFormatNone); found = true; } diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp index e137d8d9d6723..cad3be879d0e1 100644 --- a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp +++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp @@ -3087,7 +3087,7 @@ class LowerDFGToLLVM { if (!isLive(node)) { bool found = false; - if (needsOSRBackwardRewiring(node->op())) { + if (permitsOSRBackwardRewiring(node->op())) { node = node->child1().node(); if (tryToSetConstantExitArgument(exit, index, node)) return; @@ -3096,10 +3096,8 @@ class LowerDFGToLLVM { } if (!found) { - Node* int32ToDouble = 0; - Node* valueToInt32 = 0; - Node* uint32ToNumber = 0; - Node* doubleAsInt32 = 0; + Node* bestNode = 0; + unsigned bestScore = 0; HashSet::iterator iter = m_live.begin(); HashSet::iterator end = m_live.end(); @@ -3111,36 +3109,18 @@ class LowerDFGToLLVM { continue; if (candidate->child1() != node) continue; - switch (candidate->op()) { - case Int32ToDouble: - int32ToDouble = candidate; - break; - case ValueToInt32: - valueToInt32 = candidate; - break; - case UInt32ToNumber: - uint32ToNumber = candidate; - break; - case DoubleAsInt32: - uint32ToNumber = candidate; - break; - default: - ASSERT(!needsOSRForwardRewiring(candidate->op())); - break; - } + unsigned myScore = forwardRewiringSelectionScore(candidate->op()); + if (myScore <= bestScore) + continue; + bestNode = candidate; + bestScore = myScore; } - if (doubleAsInt32) - node = doubleAsInt32; - else if (int32ToDouble) - node = int32ToDouble; - else if (valueToInt32) - node = valueToInt32; - else if (uint32ToNumber) - node = uint32ToNumber; - - if (isLive(node)) + if (bestNode) { + ASSERT(isLive(bestNode)); + node = bestNode; found = true; + } } if (!found) {