Bug 96872

Summary: Structure check hoisting fails to consider the possibility of conflicting checks on the source of the first assignment to the hoisted variable
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo, ggaren, mhahnenberg, msaboff, oliver, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96907    
Bug Blocks:    
Attachments:
Description Flags
the patch oliver: review+

Filip Pizlo
Reported 2012-09-15 17:26:50 PDT
Here's the IR: a: Foo() b: SetLocal(@a, r1 c: CheckStructure(@a, <blah>) d: PutStructure(@a, <blih>) e: CheckStructure(@a, <blih>) Some other block: f: GetLocal(r1) g: CheckStructure(@a, <blih>) If we hoist the structure check for structure <blih>, we'll end up always OSR exiting since at the point of @b, the structure is still <blah>. The reason for this omission in the due diligence of structure check hoister is that while it looks at all CheckStructures on GetLocals on r1, it fails to look at CheckStructures on @a.
Attachments
the patch (8.74 KB, patch)
2012-09-15 17:34 PDT, Filip Pizlo 		oliver: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2012-09-15 17:34:58 PDT
Created attachment 164298 [details] the patch
Filip Pizlo
Comment 2 2012-09-15 19:36:55 PDT
Landed in http://trac.webkit.org/changeset/128699
Csaba Osztrogonác
Comment 3 2012-09-17 11:23:52 PDT
(In reply to comment #2) > Landed in http://trac.webkit.org/changeset/128699 It made 2 tests assert on Qt - https://bugs.webkit.org/show_bug.cgi?id=96907 Could you check it, please?
Filip Pizlo
Comment 4 2012-09-17 11:51:33 PDT
(In reply to comment #3) > (In reply to comment #2) > > Landed in http://trac.webkit.org/changeset/128699 > > It made 2 tests assert on Qt - https://bugs.webkit.org/show_bug.cgi?id=96907 > Could you check it, please? I'm on it. It's confusing though - I don't see the assertions on Mac.
Note You need to log in before you can comment on or make changes to this bug.