In https://bugs.webkit.org/show_bug.cgi?id=175470, Saam says that it is safe to remove the annotation that PushWithScope clobbers the world. This bug is just for tracking the removal of this annotation.
Created attachment 318133 [details] Patch
Comment on attachment 318133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318133&action=review > Source/JavaScriptCore/ChangeLog:7 > + It's worth a sentence describing the reasoning here. Given the semantics of PushWithScope, I think it's also worth changing Clobberize and DoesGC (based on making Clobberize not report it clobbers things). Can you also add tests that would stress a few things: - we CSE around a PushWithScope - cases where PushWithScope throws an exception in the DFG because the "o" is null/undefined - cases where PushWithScope allocates (e.g, the "o" is a number/string/Symbol)
Created attachment 318210 [details] Patch
Comment on attachment 318210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318210&action=review > Source/JavaScriptCore/dfg/DFGClobberize.h:475 > + read(ScopeProperties); This is not the case, this node does not read ScopeProperties. This heap kind is for GetClosureVar, PutClosureVar. Also, please add tests that stress the CSE this opens up.
Comment on attachment 318210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318210&action=review > Source/JavaScriptCore/dfg/DFGClobberize.h:476 > + read(HeapObjectCount); This reads more things. For example, it needs to read at least enough things to do a type check, to JSCell_typeInfo, JSCell_structureID, etc Maybe it's good to just say read(World) for now
Created attachment 318216 [details] Patch
Comment on attachment 318216 [details] Patch Clearing flags on attachment: 318216 Committed r220783: <http://trac.webkit.org/changeset/220783>
All reviewed patches have been landed. Closing bug.