Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup
Created attachment 272358 [details] Patch
Comment on attachment 272358 [details] Patch r=me
Comment on attachment 272358 [details] Patch Clearing flags on attachment: 272358 Committed r197196: <http://trac.webkit.org/changeset/197196>
All reviewed patches have been landed. Closing bug.
Comment on attachment 272358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272358&action=review > Source/JavaScriptCore/ChangeLog:3 > + Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup Why?
(In reply to comment #5) > Comment on attachment 272358 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272358&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup > > Why? When compiling a instanceof b we sometimes don't end up finding that b is a constant until after fixup. If we attempt to fold the OverridesHasInstance in fixup we won't end up removing the check when we otherwise could have.
Comment on attachment 272358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272358&action=review > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:573 > + case OverridesHasInstance: { > + if (!node->child2().node()->isCellConstant()) > + break; > + > + if (node->child2().node()->asCell() != m_graph.globalObjectFor(node->origin.semantic)->functionProtoHasInstanceSymbolFunction()) { > + m_graph.convertToConstant(node, jsBoolean(true)); > + changed = true; > + > + } else if (!m_graph.hasExitSite(node->origin.semantic, BadTypeInfoFlags)) { > + // We optimistically assume that we will not see a function that has a custom instanceof operation as they should be rare. > + m_insertionSet.insertNode(indexInBlock, SpecNone, CheckTypeInfoFlags, node->origin, OpInfo(ImplementsDefaultHasInstance), Edge(node->child1().node(), CellUse)); > + m_graph.convertToConstant(node, jsBoolean(false)); > + changed = true; > + } > + > + break; > + } > + This is not how we write constant folding rules. A constant folding rule *must* occur in DFG::AbstractInterpreterInlines if it occurs in ConstantFoldingPhase. Otherwise, the constant folding rule will have the following silly shortcomings: - It will only work in basic blocks that also incidentally did other constant folding, since you need a rule in AbstractInterpreterInlines to tell the ConstantFoldingPhase to do things in a block. - It will trigger inductive constant folding beyond the current basic block. Since AbstractInterpterInlines thinks that this node returns any boolean, other basic blocks that use this value will not know to constant-fold their uses. A simple rule to follow is: if someone writes a patch that touches ConstantFoldingPhase but not AbstractInterpreterInlines, then their patch is pretty much guaranteed to be wrong.