[Webkit-unassigned] [Bug 84598] Eliminate unnecessary SetLocal/GetLocals in DFG

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84598





--- Comment #3 from Filip Pizlo <fpizlo at apple.com>  2012-04-23 09:30:26 PST ---
(From update of attachment 138347)
View in context: https://bugs.webkit.org/attachment.cgi?id=138347&action=review

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1076
>          return;
>      for (size_t i = indexInBlock + 1; i--;)
>          forNode(m_block->at(i)).clobberStructures();
> +
> +    // Deal with the nodes defined in other blocks but referenced in current block.
> +    for (size_t arg = 0; arg < m_block->variablesForOSR.numberOfArguments(); ++arg) {
> +        NodeIndex nodeIndex = m_block->variablesForOSR.argument(arg);
> +        if (nodeIndex != NoNode)
> +            forNode(nodeIndex).clobberStructures();
> +    }
> +    for (size_t local = 0; local < m_block->variablesForOSR.numberOfLocals(); ++local) {
> +        NodeIndex nodeIndex = m_block->variablesForOSR.local(local);
> +        if (nodeIndex != NoNode)
> +            forNode(nodeIndex).clobberStructures();
> +    }
> +
>      for (size_t i = 0; i < m_variables.numberOfArguments(); ++i)
>          m_variables.argument(i).clobberStructures();
>      for (size_t i = 0; i < m_variables.numberOfLocals(); ++i)

It seems like these changes to AbstractState are not enough.

You need to make sure that each basic block has its own copy of all variables it uses, from the standpoint of AbstractState.  Otherwise you will get unsound behavior.  Example:

1: var o = ...;
2: if (p)
3:     ... = o.f;
4: ... = o.f;

AbstractState will prove in (3) that o is a cell and that it has a particular structure. But we need to be careful and make sure that we remember that there exists a path (from (2) to (4) directly, skipping (3)) where this proof has not been introduced. Hence, at (4) we actually don't know anything about o yet.

With your patch it seems that you will end up propagating the proof from (3) to (4), which is wrong.

One of the reasons why I think that SetLocal/GetLocal are useful, and why I haven't been able to convince myself to go directly to SSA, is that the SetLocal/GetLocal situation allows us to easily reason about the set of variables that are live at block boundaries, and to easy access them with a unified indexing scheme (operand numbers).  We could build similar functionality for SSA, but it will take some care.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.