Bug 19821

Summary:

Merge the instruction pair (less, jfalse)

Product:

WebKit

Reporter:

Cameron Zwarich (cpst) <zwarich>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mjs

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
SunSpider results	none
Proposed patch	oliver: review+
New SunSpider results	none

Cameron Zwarich (cpst)

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

Attachments
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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Cameron Zwarich (cpst)

Comment 1 2008-06-29 19:58:07 PDT

Created attachment 21999 [details] Patch Here's a patch that implements jnless. It has no dump code for the new opcode and the opcode documentation is wrong.

Cameron Zwarich (cpst)

Comment 2 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.

Cameron Zwarich (cpst)

Comment 3 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.

Cameron Zwarich (cpst)

Comment 4 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.

Cameron Zwarich (cpst)

Comment 5 2008-06-29 23:04:36 PDT

Created attachment 22003 [details] New SunSpider results Here are new SunSpider results, for a reference.

Oliver Hunt

Comment 6 2008-06-29 23:10:26 PDT

Comment on attachment 22002 [details] Proposed patch r=me

Cameron Zwarich (cpst)

Comment 7 2008-06-29 23:26:04 PDT

Landed in r34883.

Note You need to log in before you can comment on or make changes to this bug.