Bug 65421 - DFG JIT speculation failure pass sometimes forgets to emit code to move certain registers.
Summary: DFG JIT speculation failure pass sometimes forgets to emit code to move certa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-30 17:25 PDT by Filip Pizlo
Modified: 2011-08-01 14:39 PDT (History)
4 users (show)

See Also:


Attachments
the patch (3.62 KB, patch)
2011-07-30 17:29 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
Regression crash log (46.28 KB, text/plain)
2011-08-01 11:51 PDT, Mark Hahnenberg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-07-30 17:25:19 PDT
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.
Comment 1 Filip Pizlo 2011-07-30 17:29:31 PDT
Created attachment 102446 [details]
the patch
Comment 2 WebKit Review Bot 2011-07-30 23:14:25 PDT
Comment on attachment 102446 [details]
the patch

Clearing flags on attachment: 102446

Committed r92071: <http://trac.webkit.org/changeset/92071>
Comment 3 WebKit Review Bot 2011-07-30 23:14:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Mark Hahnenberg 2011-08-01 11:51:38 PDT
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.
Comment 5 Filip Pizlo 2011-08-01 14:39:30 PDT
(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