Bug 19821 - Merge the instruction pair (less, jfalse)
Summary: Merge the instruction pair (less, jfalse)
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2008-06-29 19:56 PDT by Cameron Zwarich (cpst)
Modified: 2008-06-29 23:26 PDT (History)
1 user (show)

See Also:

Patch (3.92 KB, patch)
2008-06-29 19:58 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
SunSpider results (3.55 KB, text/plain)
2008-06-29 20:01 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (4.94 KB, patch)
2008-06-29 23:03 PDT, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff
New SunSpider results (3.58 KB, text/plain)
2008-06-29 23:04 PDT, Cameron Zwarich (cpst)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-06-29 19:56:49 PDT
The pair (less, jfalse) is 1.61% of SunSpider instruction pairs. Merging them into a new opcode jnless is generally a win, but is a large regression on the regexp-dna and nbody tests.
Comment 1 Cameron Zwarich (cpst) 2008-06-29 19:58:07 PDT
Created attachment 21999 [details]

Here's a patch that implements jnless. It has no dump code for the new opcode and the opcode documentation is wrong.
Comment 2 Cameron Zwarich (cpst) 2008-06-29 20:01:39 PDT
Created attachment 22000 [details]
SunSpider results

Here are the SunSpider results. I'll do a before-and-after with Shark on the regexp-dna test.
Comment 3 Cameron Zwarich (cpst) 2008-06-29 22:11:25 PDT
The regression seems to be entirely caused by the CodeGenerator logic, even though the regexp-dna test doesn't hit it. I can substitute an empty opcode at the end of the opcodes in Machine.cpp and I still get the regression. If I remove the extra logic in CodeGenerator::emitJumpIfFalse, it's fine.
Comment 4 Cameron Zwarich (cpst) 2008-06-29 23:03:49 PDT
Created attachment 22002 [details]
Proposed patch

It turns out that the regression was entirely due to the changes in CodeGenerator. Adding ALWAYS_INLINE to CodeGenerator::rewindBinaryOp() fixed the regression. It is now a 2.4% progression on SunSpider.
Comment 5 Cameron Zwarich (cpst) 2008-06-29 23:04:36 PDT
Created attachment 22003 [details]
New SunSpider results

Here are new SunSpider results, for a reference.
Comment 6 Oliver Hunt 2008-06-29 23:10:26 PDT
Comment on attachment 22002 [details]
Proposed patch

Comment 7 Cameron Zwarich (cpst) 2008-06-29 23:26:04 PDT
Landed in r34883.