[JSC] In DFG, an OSR Exit on SetLocal can trash its child node
Created attachment 278130 [details] Patch
Comment on attachment 278130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278130&action=review > Source/JavaScriptCore/ChangeLog:22 > + @1 = SomethingProducingDouble() > + @2 = MovHint(@1) > + @3 = ValueRep(@1) > + @4 = SetLocal(@3, FlushedInt32) > + > + When we lower SetLocal(), we start by speculating that @3 is an Int32. > + Now this can fail if @1 was really a double. > + When that happens, we go over the VariableEventStream to find where values > + are, and @1 died at @3. Since the speculation failure happens before > + the SetLocal event, we don't do anything with @3. The bug is that the PhantomInsertionPhase didn't put a Phantom for @1 immediately after @4. Do you know why it did that? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2109 > + Node* child = node->child1().node(); > + noticeOSRBirth(child); > + m_stream->appendAndLog(VariableEvent::movHint(MinifiedID(child), node->local())); I think that this is just wrong. I see what you're trying to do, but SetLocal isn't supposed to have this kind of power. The DFG should be getting the value that was MovHinted.
Created attachment 278149 [details] Patch
I took a bit of a shortcut here: instead of querying if the Local is really killed after the current bytecode index, I assumed the local is always killed from SetLocal. I did that because it is cheaper than iterating until the next origin and getting the live locals there.
Comment on attachment 278149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278149&action=review A bit ugly, but it looks right. Damn that's a tricky corner case! > Source/JavaScriptCore/dfg/DFGPhantomInsertionPhase.cpp:142 > + auto processKilledOperand = [&] (VirtualRegister reg) > + { I think that our style is to put the { on the same line as the lambda variable. The style bot complains about this because it does not understand lambdas. But, this is the style that I have followed, because that's what I saw elsewhere in WK when I first started using them. So, let's go with that style.
Created attachment 278192 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 278192 [details]: The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 278192 [details]: transitions/default-timing-function.html bug 138901 (author: simon.fraser@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 278192 [details] Patch for landing Clearing flags on attachment: 278192 Committed r200498: <http://trac.webkit.org/changeset/200498>
All reviewed patches have been landed. Closing bug.
(In reply to comment #9) > Comment on attachment 278192 [details] > Patch for landing > > Clearing flags on attachment: 278192 > > Committed r200498: <http://trac.webkit.org/changeset/200498> It made stress/tagged-templates-template-object.js assert on Apple Mac x86 32-bit debug bots: - https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9046 - https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2305
I'll have a look.
(In reply to comment #12) > I'll have a look. The fix is here: https://bugs.webkit.org/show_bug.cgi?id=157436