96872 – Structure check hoisting fails to consider the possibility of conflicting checks on the source of the first assignment to the hoisted variable

Bug 96872 - Structure check hoisting fails to consider the possibility of conflicting checks on the source of the first assignment to the hoisted variable

Summary: Structure check hoisting fails to consider the possibility of conflicting che...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Filip Pizlo

URL:
Keywords:

Depends on:	96907
Blocks:
	Show dependency tree / graph

Reported:	2012-09-15 17:26 PDT by Filip Pizlo
Modified:	2012-09-17 11:51 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
the patch (8.74 KB, patch) 2012-09-15 17:34 PDT, Filip Pizlo	oliver: review+	Details \| Formatted Diff \| Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Filip Pizlo 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.

Comment 1 Filip Pizlo 2012-09-15 17:34:58 PDT

Created attachment 164298 [details]
the patch

Comment 2 Filip Pizlo 2012-09-15 19:36:55 PDT

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

Comment 3 Csaba Osztrogonác 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?

Comment 4 Filip Pizlo 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.