Bug 184440 – DFG AI and clobberize should agree with each other

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 337589 [details]
work in progress

[Filip Pizlo]

Created attachment 337592 [details]
the patch

[EWS Watchlist]

Attachment 337592 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3077:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/39306180>

[Saam Barati]

Comment on attachment 337592 [details]
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGClobberize.h:979
> +            read(Butterfly_publicLength);
> +            read(Butterfly_vectorLength);
> +            read(ArrayStorageProperties);
> +            write(ArrayStorageProperties);

For setters, are they always !mayStoreToHole?

> Source/JavaScriptCore/dfg/DFGClobberize.h:1331
> +        case UntypedUse:
> +            read(World);
> +            write(Heap);
> +            return;

Seems like this would be a good test to add since we don't have one.

> Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h:96
> +    // Would have the last executed node clobbered the world had we not found a way to fold it?

clobbering the world seems like the wrong comment to have here, since doing a transition doesn't necessarily entail clobbering the world, but would return true here. Maybe make the comment say something that allows for both clobbering the world and transitions?

[Filip Pizlo]

(In reply to Saam Barati from comment #5)
> Comment on attachment 337592 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337592&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGClobberize.h:979
> > +            read(Butterfly_publicLength);
> > +            read(Butterfly_vectorLength);
> > +            read(ArrayStorageProperties);
> > +            write(ArrayStorageProperties);
> 
> For setters, are they always !mayStoreToHole?

SlowPutArrayStorage means you may have accessors. So, storing to a hole means possibly calling an accessor.

> 
> > Source/JavaScriptCore/dfg/DFGClobberize.h:1331
> > +        case UntypedUse:
> > +            read(World);
> > +            write(Heap);
> > +            return;
> 
> Seems like this would be a good test to add since we don't have one.

This is already covered by a test thanks to the new assertion!

> 
> > Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h:96
> > +    // Would have the last executed node clobbered the world had we not found a way to fold it?
> 
> clobbering the world seems like the wrong comment to have here, since doing
> a transition doesn't necessarily entail clobbering the world, but would
> return true here. Maybe make the comment say something that allows for both
> clobbering the world and transitions?

I made it say "clobbered things".  This returns true if the node clobbers anything that the AI knows about how to clobber, which currently just means structures.

[Filip Pizlo]

I think that it would be best if I wrote a test case for all of the cases that I found even though the new assertion gives us such cases for free.  I'll do that and then land.

[Filip Pizlo]

Created attachment 337621 [details]
patch for landing

[EWS Watchlist]

Attachment 337621 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3075:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/230488/webkit

[Ryan Haddad]

This change broke the CLoop build: https://build.webkit.org/builders/Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/5032

./dfg/DFGAbstractInterpreterClobberState.cpp:33:38: error: use of undeclared identifier 'JSC'

[Filip Pizlo]

(In reply to Ryan Haddad from comment #11)
> This change broke the CLoop build:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/5032
> 
> ./dfg/DFGAbstractInterpreterClobberState.cpp:33:38: error: use of undeclared
> identifier 'JSC'

Fixing...

[Filip Pizlo]

Fixed in r230494.