From fdd18f144baf4f2049dfc5a090ac9a38d75e6548 Mon Sep 17 00:00:00 2001 From: "fpizlo@apple.com" Date: Sun, 15 Sep 2013 00:57:42 +0000 Subject: [PATCH] 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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@155793 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/JavaScriptCore/ChangeLog | 75 +++++++++++++++++++ Source/JavaScriptCore/dfg/DFGMinifiedNode.h | 2 +- Source/JavaScriptCore/dfg/DFGNodeType.h | 57 ++++++++++++-- .../dfg/DFGVariableEventStream.cpp | 68 ++++++----------- .../JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp | 44 +++-------- 5 files changed, 159 insertions(+), 87 deletions(-) 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) {