Bug 101438 - DFG should not assume that something is a double just because it might be undefined
Summary: DFG should not assume that something is a double just because it might be und...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-07 01:28 PST by Filip Pizlo
Modified: 2012-11-08 14:56 PST (History)
7 users (show)

See Also:


Attachments
the patch (23.90 KB, patch)
2012-11-07 01:36 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (23.71 KB, patch)
2012-11-07 02:00 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-07 01:28:39 PST
Currently our arithmetic paths in the DFG have two, or three, versions:

1) Integer path.  This only accepts integers.

2) Double path.  This only accepts doubles or integers.

3) For ValueAdd only, there is a generic path.

For operations that only have (1) or (2), we currently assume that we must use (2) if either operand may not be a number.  This is clearly pointless, as it means that we do an upconversion to double even when we have no evidence that the value is in fact a double.

For ValueAdd, it probably makes no sense to fall off into the generic path just because the value can be undefined.  Undefined data flow arcs may exist if a variable is conditionally assigned, like in:

var x;
if (p)
   x = 42;
... // stuff
if (p)
   y = x + 1; // Currently we'll use generic ValueAdd here.

We should change the speculation logic to account for the fact that (a) undefined flowing into arithmetic means we've probably already lost anyway and (b) speculating double for something that is either integer or not even a number buys nothing.
Comment 1 Filip Pizlo 2012-11-07 01:36:45 PST
Created attachment 172738 [details]
the patch
Comment 2 WebKit Review Bot 2012-11-07 01:41:37 PST
Attachment 172738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:319:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:360:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:384:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:822:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2370:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2407:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:266:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:284:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2012-11-07 02:00:36 PST
Created attachment 172740 [details]
the patch

Fix style
Comment 4 Filip Pizlo 2012-11-08 14:56:21 PST
Landed in http://trac.webkit.org/changeset/133956