Patch forthcoming.
Created attachment 337589 [details] work in progress
Created attachment 337592 [details] the patch
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.
<rdar://problem/39306180>
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?
(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.
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.
Created attachment 337621 [details] patch for landing
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.
Landed in https://trac.webkit.org/changeset/230488/webkit
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'
(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...
Fixed in r230494.