Bug 103353 – DFG SetLocal should use forwardSpeculationCheck instead of its own half-baked version of same

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

This will ensure correct behavior if (a) the bytecode op that the SetLocal belongs to is side-effecting, (b) the SetLocal does speculations, and (c) the SetLocal has some other node after it for the same bytecode op (which will happen for intrinsic calls).

[Filip Pizlo]

<rdar://problem/12734106>

[Filip Pizlo]

Created attachment 176158 [details]
the patch

[Gavin Barraclough]

Comment on attachment 176158 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2327
> +            // And twice for real.

This may be non-obvious, and it's possible the idiom required to make this work could change.
I wonder if we should have a helper to set two registers, e.g.:
    setPair(result, op, srcDst, resultIndex)
This could internally perform the four set() calls (as this is currently implemented).

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 176158 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176158&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2327
> > +            // And twice for real.
> 
> This may be non-obvious, and it's possible the idiom required to make this work could change.
> I wonder if we should have a helper to set two registers, e.g.:
>     setPair(result, op, srcDst, resultIndex)
> This could internally perform the four set() calls (as this is currently implemented).

Good call!  I will land with this change unless there are objections.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/135923