[webkit-reviews] review denied: [Bug 98898] Partial support of 'typeof' node in DFG : [Attachment 167992] patch with fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 10 13:31:47 PDT 2012


Filip Pizlo <fpizlo at apple.com> has denied Valery Ignatyev
<valery.ignatyev at ispras.ru>'s request for review:
Bug 98898: Partial support of 'typeof' node in DFG
https://bugs.webkit.org/show_bug.cgi?id=98898

Attachment 167992: patch with fix
https://bugs.webkit.org/attachment.cgi?id=167992&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.


More information about the webkit-reviews mailing list