[webkit-reviews] review denied: [Bug 174352] [FTL] Arguments elimination is suppressed by unreachable blocks : [Attachment 315410] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 174352: [FTL] Arguments elimination is suppressed by unreachable blocks
https://bugs.webkit.org/show_bug.cgi?id=174352

Attachment 315410: Patch

https://bugs.webkit.org/attachment.cgi?id=315410&action=review




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 315410
  --> https://bugs.webkit.org/attachment.cgi?id=315410
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315410&action=review

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:414
> +		   case GetById:
> +		       // If we already perform OSR-exit, GetById can be simply
ignored.
> +		       // This can happen if you have a basic block that is not
executed yet and references elimination candidates.
> +		       // Since this GetById code is not executed yet, it is
always GetById (neither GetByOffset nor GetByIdFlush).
> +		       // We do not need to consider GetByIdFlush here.
> +		       // We focus on ForceOSRExit. It is OK because explicit
OSR exit nodes like Unreachable, Throw etc. are terminal
> +		       // nodes. So it does not appear in the middle of a basic
block.
> +		       if (isOSRExited)
> +			   break;
> +		       m_graph.doToChildren(node, [&] (Edge edge) { return
escape(edge, node); });
> +		       break;

We can't just special case GetById. There are plenty of other operations that
might escape an allocation.

Regarding your boolean here, why not just break out of walking over this block
when seeing a node that is an exit.
I would just add something like:
case ForceOSRExit:
case CheckBadCell:
case Throw:
case ThrowStaticError:
    break out of for loop.

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:1118
> +		   isOSRExited |= node->isPseudoTerminal();

and then I'd just add the same cases to this switch statement, and break out
early.

> Source/JavaScriptCore/dfg/DFGNode.h:1313
> +    // As is described in DFGNodeType.h's ForceOSRExit, this is a
pseudo-terminal.
> +    // It means that execution should fall out of DFG at this point, but
execution
> +    // does continue in the basic block - just in a different compiler.
> +    bool isPseudoTerminal()

I wouldn't add this function. This is nicer just in the arguments elimination
phase as part of its switch statements.

> Source/JavaScriptCore/dfg/DFGValidate.cpp:737
> +		   case GetById:
> +		       if (isOSRExited)
> +			   break;
> +		       FALLTHROUGH;

ditto here.