Bug 100461 - Forward OSR calculation is wrong in the presence of multiple SetLocals, or a mix of SetLocals and Phantoms
Summary: Forward OSR calculation is wrong in the presence of multiple SetLocals, or a ...
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-10-25 22:34 PDT by Filip Pizlo
Modified: 2012-10-26 15:19 PDT (History)
7 users (show)

See Also:


Attachments
the patch (12.02 KB, patch)
2012-10-25 23:00 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (12.72 KB, patch)
2012-10-26 14:40 PDT, 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-10-25 22:34:43 PDT
This was broken by the attempt to have the forward OSR exit calculator collect all SetLocals and create recovery overrides for them.  The fix is to not do that, and instead have bytecode ops that decompose into multiple SetLocals explicitly help the forward OSR exit calculator with SetLocal hints.

<rdar://problem/12551946>
Comment 1 Filip Pizlo 2012-10-25 23:00:23 PDT
Created attachment 170822 [details]
the patch
Comment 2 Filip Pizlo 2012-10-25 23:01:03 PDT
Comment on attachment 170822 [details]
the patch

Clearing cq? because I will probably want to land this with a test.  I would appreciate a review even though I haven't had a chance to try to write the test, yet.  Doing so will be tricky.
Comment 3 Oliver Hunt 2012-10-26 07:52:55 PDT
Comment on attachment 170822 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3118
> +                // First create OSR hints only.
> +                set(baseDst, base);
> +                set(valueDst, value);
> +                
> +                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
> +                // *after* we have already set up the values for OSR.
> +                
> +                // Then do the real SetLocals.

Don't we need this for resolve_with_this as well?
Comment 4 Filip Pizlo 2012-10-26 14:34:33 PDT
(In reply to comment #3)
> (From update of attachment 170822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170822&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3118
> > +                // First create OSR hints only.
> > +                set(baseDst, base);
> > +                set(valueDst, value);
> > +                
> > +                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
> > +                // *after* we have already set up the values for OSR.
> > +                
> > +                // Then do the real SetLocals.
> 
> Don't we need this for resolve_with_this as well?

Ooops.
Comment 5 Filip Pizlo 2012-10-26 14:40:55 PDT
Created attachment 171016 [details]
the patch
Comment 6 Filip Pizlo 2012-10-26 15:19:30 PDT
Landed in http://trac.webkit.org/changeset/132701