It is by design that we don't support sinking of function allocation through any related operations. Object allocation, however, does sink through field assignment (PutByOffset etc.), so we have a check that is supposed to prevent function allocations from sinking through these. However that check is misguided and (a) do not prevent function allocation sinking through field assignment and (b) ensures that if a function allocation ever sinks through a PutByOffset, very bad things happen instead. Fortunately, PutByOffset (and their relatives) require first allocating a custom structure for the JSFunction object, thus the PutByOffset will always be preceded by an AllocatePropertyStorage through which no allocation can sink, and materialization would always be triggered before the PutByOffset - so this works by chance. Anyway, this should be done the proper way, and I will submit a patch soon to prevent properly have checks preventing function allocation sinking through PutByOffset et al.
Created attachment 251602 [details] Should do the right thing Compiles, but untested - will check my understanding of the nodes is correct and does not prevent valid object allocation sinking and add a ChangeLog entry on Monday.
Created attachment 251763 [details] Seems to be doing the right thing
Created attachment 251764 [details] Seems to be doing the right thing
Comment on attachment 251764 [details] Seems to be doing the right thing View in context: https://bugs.webkit.org/attachment.cgi?id=251764&action=review > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812 > + if (target->op() != NewObject) Unsure if I should add a helper function in DFG::Node for that...
You should rebase :-)
Comment on attachment 251764 [details] Seems to be doing the right thing View in context: https://bugs.webkit.org/attachment.cgi?id=251764&action=review >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812 >> + if (target->op() != NewObject) > > Unsure if I should add a helper function in DFG::Node for that... Seems wrong - MaterializeNewObject is also a sink candidate.
(In reply to comment #6) > Comment on attachment 251764 [details] > Seems to be doing the right thing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251764&action=review > > >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812 > >> + if (target->op() != NewObject) > > > > Unsure if I should add a helper function in DFG::Node for that... > > Seems wrong - MaterializeNewObject is also a sink candidate. Oh, duh, indeed. Fixing that, w/ a helper function then.
Created attachment 251768 [details] MaterializeNewObject is also sinkable...
Sorry, should have tried a clean build.
Created attachment 251771 [details] Remove spurious MustGenerate
Comment on attachment 251771 [details] Remove spurious MustGenerate Clearing flags on attachment: 251771 Committed r183419: <http://trac.webkit.org/changeset/183419>
All reviewed patches have been landed. Closing bug.