[Webkit-unassigned] [Bug 98898] Partial support of 'typeof' node in DFG
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 10 13:31:48 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=98898
Filip Pizlo <fpizlo at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #167992|review? |review-
Flag| |
--- Comment #4 from Filip Pizlo <fpizlo at apple.com> 2012-10-10 13:32:27 PST ---
(From update of attachment 167992)
View in context: https://bugs.webkit.org/attachment.cgi?id=167992&action=review
> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1545
> + case Typeof:
> + node.setCanExit(true);
> + m_foundConstants = true;
> + break;
This should at the *very least* say forNode(nodeIndex).set(SpecString). At this point you have no knowledge of what Typeof will return.
And as I say in comments in ConstantFolding, this should do type casing to see if we can prove Typeof away.
> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2941
> + NEXT_OPCODE(op_new_func_exp);
This should be NEXT_OPCODE(op_new_func_exp)
> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:114
> + SpeculatedType child = m_graph[node.child1()].prediction();
You meant m_state.forNode(node.child1()).m_type.
The prediction is unsound - it's just what we *thing* that the node *might* be, rather than what we *know* the node *to* be.
If you want to optimize Typeof based on predictions, then it cause Typeof to get compiled into a speculation check followed by the relevant constant.
> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:126
> + if (isObjectSpeculation(child))
> + value = JSValue(jsString(&m_graph.m_globalData, String("object")));
> + else if (isNumberSpeculation(child))
> + value = JSValue(jsString(&m_graph.m_globalData, String("number")));
> + else if (isStringSpeculation(child))
> + value = JSValue(jsString(&m_graph.m_globalData, String("string")));
> + else if (isFunctionSpeculation(child))
> + value = JSValue(jsString(&m_graph.m_globalData, String("function")));
> + else if (isBooleanSpeculation(child))
> + value = JSValue(jsString(&m_graph.m_globalData, String("boolean")));
This does not belong here. Prediction-based optimizations should be done in Fixup rather than ConstantFolding. But, as I say above, the fact that you're doing a prediction-based optimization is probably wrong.
You should take this code and move it to AbstractState so that the CFA can fixpoint on it.
Probably the right thing is to have both:
1) If the predictions say Blah then Fixup should emit a node to speculate Blah and then emit the constant node in place of the Typeof.
2) If the proofs (i.e. AbstractState) say Blah then the AbstractState will use logic like the thing you have above to mark the Typeof node as being known-constant, and then constant folding will just do the right things.
> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:133
> + Node foe(ForceOSRExit, node.codeOrigin);
> + foe.setRefCount(1);
> + NodeIndex foeNodeIndex = m_graph.size();
> + m_graph.append(foe);
> + m_insertionSet.append(indexInBlock - 1, foeNodeIndex);
> + break;
I don't like this at all. Why not just have the Typeof node call a C function if it's not optimized out?
> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:188
> + Node& child1 = m_graph[node.child1()];
> + if (child1.op() == GetLocal) {
> + NodeIndex previousLocalAccess = NoNode;
> + if (block->variablesAtHead.operand(child1.local()) == node.child1()
> + && m_graph[child1.child1()].op() == Phi) {
> + // We expect this to be the common case.
> + ASSERT(block->isInPhis(child1.child1().index()));
> + previousLocalAccess = child1.child1().index();
> + block->variablesAtHead.operand(child1.local()) = previousLocalAccess;
> + } else {
> + ASSERT(indexInBlock - 1 > 0);
> + // Must search for the previous access to this local.
> + for (BlockIndex subIndexInBlock = indexInBlock - 1 ; subIndexInBlock--;) {
> + NodeIndex subNodeIndex = block->at(subIndexInBlock);
> + Node& subNode = m_graph[subNodeIndex];
> + if (!subNode.shouldGenerate())
> + continue;
> + if (!subNode.hasVariableAccessData())
> + continue;
> + if (subNode.local() != child1.local())
> + continue;
> + // The two must have been unified.
> + ASSERT(subNode.variableAccessData() == child1.variableAccessData());
> + previousLocalAccess = subNodeIndex;
> + break;
> + }
> + if (previousLocalAccess == NoNode) {
> + // The previous access must have been a Phi.
> + for (BlockIndex phiIndexInBlock = block->phis.size(); phiIndexInBlock--;) {
> + NodeIndex phiNodeIndex = block->phis[phiIndexInBlock];
> + Node& phiNode = m_graph[phiNodeIndex];
> + if (!phiNode.shouldGenerate())
> + continue;
> + if (phiNode.local() != child1.local())
> + continue;
> + // The two must have been unified.
> + ASSERT(phiNode.variableAccessData() == child1.variableAccessData());
> + previousLocalAccess = phiNodeIndex;
> + break;
> + }
> + ASSERT(previousLocalAccess != NoNode);
> + }
> + }
> +
> + ASSERT(previousLocalAccess != NoNode);
> + NodeIndex tailNodeIndex = block->variablesAtTail.operand(child1.local());
> + if (tailNodeIndex == node.child1())
> + block->variablesAtTail.operand(child1.local()) = previousLocalAccess;
> + else {
> + ASSERT(m_graph[tailNodeIndex].op() == Flush);
> + ASSERT(m_graph[tailNodeIndex].op() == SetLocal);
> + }
> + child1.setOpAndDefaultFlags(Phantom);
Are you duplicating code here? Why?
And why are you killing the GetLocal child of the Typeof node?
> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:689
> + }
> + break;
break; should be inside the { }.
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4287
> + case Typeof:
> + ASSERT_NOT_REACHED();
> + break;
As I said in my comments in ConstantFolding, the fall back for Typeof should be a procedure call.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list