Bug 20912

Summary:

Use simple int32 multiplication on op_mul if it is possible

Product:

WebKit

Reporter:

Gabor Loki <loki>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

abarth, zwarich

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
int32 multiplication on op_mul	none
SunSpider, V8 results (rev 36540)	none
int32 multiplication on op_mul (v2)	zwarich: review-
int32 multiplication on op_mul (v3)	zwarich: review+
SunSpider, V8 results (rev 37601)	none

Gabor Loki

Reported 2008-09-18 04:02:36 PDT

We can use int32 multiplication instead of double one on op_mul if both operands are immediate and they are between 0 - 0x7FFFF.

Attachments
int32 multiplication on op_mul (1.59 KB, patch) 2008-09-18 04:04 PDT, Gabor Loki	no flags	Details Formatted Diff Diff
SunSpider, V8 results (rev 36540) (4.43 KB, text/plain) 2008-09-18 04:09 PDT, Gabor Loki	no flags	Details
int32 multiplication on op_mul (v2) (1.53 KB, patch) 2008-09-26 03:52 PDT, Gabor Loki	zwarich: review-	Details Formatted Diff Diff
int32 multiplication on op_mul (v3) (1.72 KB, patch) 2008-10-15 02:13 PDT, Gabor Loki	zwarich: review+	Details Formatted Diff Diff
SunSpider, V8 results (rev 37601) (4.39 KB, text/plain) 2008-10-15 02:15 PDT, Gabor Loki	no flags	Details
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Gabor Loki

Comment 1 2008-09-18 04:04:42 PDT

Created attachment 23525 [details] int32 multiplication on op_mul

Gabor Loki

Comment 2 2008-09-18 04:09:49 PDT

Created attachment 23526 [details] SunSpider, V8 results (rev 36540) The progression is 2.5% on SunSpider, and 7.8% on V8. Would someone be so kind to measure it on Mac (without CTI)?

Maciej Stachowiak

Comment 3 2008-09-18 23:23:51 PDT

Comment on attachment 23525 [details] int32 multiplication on op_mul r=me

Gabor Loki

Comment 4 2008-09-26 03:52:11 PDT

Created attachment 23846 [details] int32 multiplication on op_mul (v2) The previous patch has a review+ flag for a week, but nobody landed. So, I have did a small tweak and updated the patch. The current results are the followings: SunSpider: 1.5% speedup, V8: 8.7% speedup

Maciej Stachowiak

Comment 5 2008-09-26 09:20:28 PDT

Comment on attachment 23846 [details] int32 multiplication on op_mul (v2) r=me (again)

Adam Barth

Comment 6 2008-10-14 01:30:03 PDT

Will land v2. This is a bit outside of my area of expertise but looks self-contained. Please stop me if I'm overstepping.

Adam Barth

Comment 7 2008-10-14 03:01:06 PDT

My Mac Mini died in the middle of testing this patch. I'm going to take it into the store tomorrow.

Adam Barth

Comment 8 2008-10-14 12:27:48 PDT

It's going to be a week before I get my Mac Mini back. Unassigning. This patch needs to be tweaked before landing, but the changes are obvious.

Cameron Zwarich (cpst)

Comment 9 2008-10-14 13:30:14 PDT

Comment on attachment 23846 [details] int32 multiplication on op_mul (v2) If I try this patch on ToT (with r and exec changed to callFrame), I get 168 JSC test regressions.

Cameron Zwarich (cpst)

Comment 10 2008-10-14 13:31:48 PDT

Any new version of this patch should also use C++ style casts instead of C style casts.

Gabor Loki

Comment 11 2008-10-15 01:30:43 PDT

> If I try this patch on ToT (with r and exec changed to callFrame), I get 168 > JSC test regressions. It can't be. I guess you forgot to update VPC's statement. I will send the updated patch and its results on Qt-linux soon.

Gabor Loki

Comment 12 2008-10-15 02:13:30 PDT

Created attachment 24354 [details] int32 multiplication on op_mul (v3) Here is the updated patch on ToT. No new regression in JSC tests.

Gabor Loki

Comment 13 2008-10-15 02:15:48 PDT

Created attachment 24355 [details] SunSpider, V8 results (rev 37601) The progression is 3.1% on SunSpider, and 10.6% on V8 (on Qt-linux).

Cameron Zwarich (cpst)

Comment 14 2008-10-15 02:24:21 PDT

Comment on attachment 24354 [details] int32 multiplication on op_mul (v3) r=me

Cameron Zwarich (cpst)

Comment 15 2008-10-15 02:36:47 PDT

Landed in r37602. Sorry about that, I should have been able to restructure the patch myself.

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