Bug 103353 - DFG SetLocal should use forwardSpeculationCheck instead of its own half-baked version of same
Summary: DFG SetLocal should use forwardSpeculationCheck instead of its own half-baked...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-11-26 20:49 PST by Filip Pizlo
Modified: 2012-11-27 14:35 PST (History)
7 users (show)

See Also:


Attachments
the patch (89.58 KB, patch)
2012-11-26 20:54 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-26 20:49:19 PST
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).
Comment 1 Filip Pizlo 2012-11-26 20:49:48 PST
<rdar://problem/12734106>
Comment 2 Filip Pizlo 2012-11-26 20:54:12 PST
Created attachment 176158 [details]
the patch
Comment 3 Gavin Barraclough 2012-11-26 22:36:13 PST
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).
Comment 4 Filip Pizlo 2012-11-26 22:49:12 PST
(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.
Comment 5 Filip Pizlo 2012-11-27 14:35:59 PST
Landed in http://trac.webkit.org/changeset/135923