Bug 154743 – Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup

[Keith Miller]

Created attachment 272358 [details]
Patch

[Mark Lam]

Comment on attachment 272358 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 272358 [details]
Patch

Clearing flags on attachment: 272358

Committed r197196: <http://trac.webkit.org/changeset/197196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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?

[Keith Miller]

(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.

[Filip Pizlo]

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.