Bug 64573 – DFG JIT is inconsistent about fusing branches and speculating integer comparisons for branches

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 100902 [details]
the patch

[WebKit Review Bot]

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.

[Filip Pizlo]

Created attachment 100908 [details]
the patch (fix style)

[Gavin Barraclough]

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.

[WebKit Review Bot]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[WebKit Review Bot]

Comment on attachment 100942 [details]
the patch (fix Platform.h)

Clearing flags on attachment: 100942

Committed r91099: <http://trac.webkit.org/changeset/91099>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.