Bug 65421 – DFG JIT speculation failure pass sometimes forgets to emit code to move certain registers.

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

[Filip Pizlo]

The DFG speculation failure pass attempts to be completely optimal.  Part of this strategy is that if a register is either already spilled on the speculative path, or not spilled on the non-speculative path, then it does not need to be spilled.  However, as part of further optimizations, the same loop that detects when to spill also sets up data structures for moving registers around (such as if node X is in register A on the speculative path and in register B on the non-speculative path, necessitating a move).  Unfortunately, this loop is badly formed: it will skip over a register entirely if it is already spilled on the speculative path, rather than just not spilling it.  This means that if a register is spilled on the speculative path, and needs to be moved to a different register on the non-speculative path, the move will never happen resulting in badness if speculation fails.

This crash is responsible for docs.google.com spreadsheets not loading.

[Filip Pizlo]

Created attachment 102446 [details]
the patch

[WebKit Review Bot]

Comment on attachment 102446 [details]
the patch

Clearing flags on attachment: 102446

Committed r92071: <http://trac.webkit.org/changeset/92071>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Hahnenberg]

Created attachment 102540 [details]
Regression crash log

I think this patch caused lots of regressions in the layout tests.  When running the layout tests, DumpRenderTree usually crashes in one of two ways--either due to a GC issue or, if the tests are run a second time, a failed assertion at DFG::needDataFormatConversion(JSC::DFG::DataFormat, JSC::DFG::DataFormat) + 96 (DFGGenerationInfo.h:54).  Crash log for the latter case is attached.

[Filip Pizlo]

(In reply to comment #4)
> Created an attachment (id=102540) [details]
> Regression crash log
> 
> I think this patch caused lots of regressions in the layout tests.  When running the layout tests, DumpRenderTree usually crashes in one of two ways--either due to a GC issue or, if the tests are run a second time, a failed assertion at DFG::needDataFormatConversion(JSC::DFG::DataFormat, JSC::DFG::DataFormat) + 96 (DFGGenerationInfo.h:54).  Crash log for the latter case is attached.

Oh yeah, that's a bug. :-)  Oddly it wasn't created by this patch, it was just "revealed" by this patch.  Previous implementations of the code that this patch addresses were totally broken but worked by accident and happened to never hit the part where a register wasn't correctly formatted.  See https://bugs.webkit.org/show_bug.cgi?id=65490