Bug 64573 - DFG JIT is inconsistent about fusing branches and speculating integer comparisons for branches
Summary: DFG JIT is inconsistent about fusing branches and speculating integer compari...
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-14 16:40 PDT by Filip Pizlo
Modified: 2011-07-15 13:14 PDT (History)
2 users (show)

See Also:


Attachments
the patch (19.58 KB, patch)
2011-07-14 17:28 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix style) (19.03 KB, patch)
2011-07-14 18:14 PDT, Filip Pizlo
barraclough: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
the patch (fix conflict, fix review) (19.44 KB, patch)
2011-07-15 00:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix Platform.h) (18.89 KB, patch)
2011-07-15 00:25 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-07-14 16:40:58 PDT
The DFG speculative JIT typically speculates that compare operands are integers, except when they are fused branches; in which case it might always perform a slow call.  The DFG speculative JIT typically fuses compare/branch sequences, except for StringEqual.  The non-speculative JIT always first tries integer comparisons, and never goes straight to a C call for a comparison, but it never fuses branches.  It would be better if both JITs had a common strategy for compare/branch fusion, and if the speculative JIT always first attempted a fast comparison before going to slow calls.
Comment 1 Filip Pizlo 2011-07-14 17:28:24 PDT
Created attachment 100902 [details]
the patch
Comment 2 WebKit Review Bot 2011-07-14 17:52:11 PDT
Attachment 100902 [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/DFGOperations.cpp:424:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:425:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2011-07-14 18:14:22 PDT
Created attachment 100908 [details]
the patch (fix style)
Comment 4 Gavin Barraclough 2011-07-14 22:34:37 PDT
Comment on attachment 100908 [details]
the patch (fix style)

static_cast<int32_t>(1) seems a little odd... I guess this is just code that's moving (I guess we could also just return a JSValue / intptr_t from these helper operations, save the extra instruction).  No need for these ideas to be incorporated into this patch though.
Comment 5 WebKit Review Bot 2011-07-14 22:38:48 PDT
Comment on attachment 100908 [details]
the patch (fix style)

Rejecting attachment 100908 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
2 FAILED at 534.
1 out of 2 hunks FAILED -- saving rejects to file Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h.rej
patching file Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp
patching file Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.h
patching file Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
patching file Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Gavin Barraclough', u'..." exit_code: 1

Full output: http://queues.webkit.org/results/9093107
Comment 6 Filip Pizlo 2011-07-14 23:25:37 PDT
(In reply to comment #4)
> (From update of attachment 100908 [details])
> static_cast<int32_t>(1) seems a little odd... I guess this is just code that's moving (I guess we could also just return a JSValue / intptr_t from these helper operations, save the extra instruction).  No need for these ideas to be incorporated into this patch though.

Yeah, that looks like a wart.  I'l go ahead and fix it, since it appears that this patch failed to land.
Comment 7 Filip Pizlo 2011-07-15 00:17:27 PDT
Created attachment 100941 [details]
the patch (fix conflict, fix review)

Fixes a conflict with one of my other patches.  This patch should be able to land successfully.  Also removes the superfluous static_cast.
Comment 8 Filip Pizlo 2011-07-15 00:25:36 PDT
Created attachment 100942 [details]
the patch (fix Platform.h)

Oops - the last patch included a change to Platform.h that shouldn't have gotten into the patch.
Comment 9 WebKit Review Bot 2011-07-15 13:14:40 PDT
Comment on attachment 100942 [details]
the patch (fix Platform.h)

Clearing flags on attachment: 100942

Committed r91099: <http://trac.webkit.org/changeset/91099>
Comment 10 WebKit Review Bot 2011-07-15 13:14:45 PDT
All reviewed patches have been landed.  Closing bug.