Bug 101438

Summary: DFG should not assume that something is a double just because it might be undefined
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch oliver: review+

Filip Pizlo
Reported 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.
Attachments
the patch (23.90 KB, patch)
2012-11-07 01:36 PST, Filip Pizlo
no flags
the patch (23.71 KB, patch)
2012-11-07 02:00 PST, Filip Pizlo
oliver: review+
Filip Pizlo
Comment 1 2012-11-07 01:36:45 PST
Created attachment 172738 [details] the patch
WebKit Review Bot
Comment 2 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.
Filip Pizlo
Comment 3 2012-11-07 02:00:36 PST
Created attachment 172740 [details] the patch Fix style
Filip Pizlo
Comment 4 2012-11-08 14:56:21 PST
Note You need to log in before you can comment on or make changes to this bug.