Bug 154743

Summary: Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Keith Miller 2016-02-26 12:46:22 PST
Folding of OverridesHasInstance DFG nodes shoud happen in constant folding not fixup
Comment 1 Keith Miller 2016-02-26 12:47:34 PST
Created attachment 272358 [details]
Patch
Comment 2 Mark Lam 2016-02-26 12:52:39 PST
Comment on attachment 272358 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2016-02-26 13:43:04 PST
Comment on attachment 272358 [details]
Patch

Clearing flags on attachment: 272358

Committed r197196: <http://trac.webkit.org/changeset/197196>
Comment 4 WebKit Commit Bot 2016-02-26 13:43:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Geoffrey Garen 2016-02-26 14:12:45 PST
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?
Comment 6 Keith Miller 2016-02-26 14:37:44 PST
(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 7 Filip Pizlo 2016-02-27 15:04:12 PST
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.