Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(231)

Issue 22600005: Eliminate intentional conversion from Smi to Int32 in HMul (Closed)

Created:
7 years, 4 months ago by weiliang.lin2
Modified:
7 years, 3 months ago
Reviewers:
Toon Verwaest
CC:
v8-dev
Base URL:
https://github.com/v8/v8.git@master
Visibility:
Public.

Description

Eliminate intentional conversion from Smi to Int32 in HMul If not all uses of arithmetic binary operation can be truncated to Smi, check if they can be truncated to Int32 which could avoid minus zero check Fixed DoMulI on X64 to adopt correct operand size when the representation is Smi Fixed DoMulI on ARM. Constant right operand optimization is based on Integer 32 instead of its representation. BUG= R=verwaest@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=16361

Patch Set 1 #

Total comments: 2

Patch Set 2 : merge HMul/smi issues together #

Patch Set 3 : Addressed patch set 1 comment #

Patch Set 4 : Fixed ARM failures #

Total comments: 4

Patch Set 5 : Use separate worklists for smi and int32 to compute truncation flag for phis #

Patch Set 6 : Fixed navier stokes benchmark fails #

Unified diffs Side-by-side diffs Delta from patch set Stats (+161 lines, -65 lines) Patch
M src/arm/lithium-codegen-arm.cc View 1 2 3 4 2 chunks +2 lines, -5 lines 0 comments Download
M src/hydrogen-canonicalize.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M src/hydrogen-infer-representation.cc View 1 2 3 4 5 1 chunk +22 lines, -10 lines 0 comments Download
M src/hydrogen-instructions.h View 1 2 3 4 5 5 chunks +8 lines, -2 lines 0 comments Download
M src/hydrogen-instructions.cc View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
M src/hydrogen-representation-changes.cc View 1 2 3 4 2 chunks +42 lines, -28 lines 0 comments Download
M src/runtime.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/runtime.cc View 1 1 chunk +13 lines, -0 lines 0 comments Download
M src/x64/lithium-codegen-x64.cc View 1 2 3 4 4 chunks +24 lines, -6 lines 0 comments Download
A + test/mjsunit/smi-mul.js View 1 1 chunk +32 lines, -13 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
weiliang.lin2
Elimination intentional conversion from Smi to Int32 in HMul could lead to regress-2132.js failure on ...
7 years, 4 months ago (2013-08-08 15:02:17 UTC) #1
Sven Panne
Test cases, please... :-)
7 years, 4 months ago (2013-08-09 05:56:23 UTC) #2
weiliang.lin2
On 2013/08/09 05:56:23, Sven Panne wrote: > Test cases, please... :-) Hi Sven, test case ...
7 years, 4 months ago (2013-08-09 06:00:14 UTC) #3
Sven Panne
On 2013/08/09 06:00:14, weiliang.lin2 wrote: > On 2013/08/09 05:56:23, Sven Panne wrote: > > Test ...
7 years, 4 months ago (2013-08-09 06:03:01 UTC) #4
weiliang.lin2
On 2013/08/09 06:03:01, Sven Panne wrote: > On 2013/08/09 06:00:14, weiliang.lin2 wrote: > > On ...
7 years, 4 months ago (2013-08-09 12:54:21 UTC) #5
Toon Verwaest
One comment so far, but could you please merge both HMul/smi related issues together? They ...
7 years, 4 months ago (2013-08-09 14:10:13 UTC) #6
weiliang.lin2
HMul/smi related issues are merged here. Now, ia32/x64 are done. For ARM platform, there are ...
7 years, 4 months ago (2013-08-09 15:09:50 UTC) #7
weiliang.lin2
Hi Toon The CL is ready. Please review it. Thanks
7 years, 4 months ago (2013-08-10 13:51:26 UTC) #8
Toon Verwaest
Looking good, but we should fix up the algorithm that annotates phis with truncation information. ...
7 years, 4 months ago (2013-08-10 14:13:19 UTC) #9
weiliang.lin2
Yes, that algorithm should be modified based on your comments. Thanks a lot~ https://codereview.chromium.org/22600005/diff/15001/src/hydrogen-representation-changes.cc File ...
7 years, 4 months ago (2013-08-10 16:43:26 UTC) #10
Toon Verwaest
Octane's navier stokes benchmark fails with the current implementation. Enabling smi for HMul also seems ...
7 years, 4 months ago (2013-08-11 16:11:49 UTC) #11
weiliang.lin2
On 2013/08/11 16:11:49, Toon Verwaest wrote: > Octane's navier stokes benchmark fails with the current ...
7 years, 4 months ago (2013-08-12 06:24:15 UTC) #12
weiliang.lin2
On 2013/08/12 06:24:15, weiliang.lin2 wrote: > On 2013/08/11 16:11:49, Toon Verwaest wrote: > > Octane's ...
7 years, 4 months ago (2013-08-12 14:36:45 UTC) #13
weiliang.lin2
On 2013/08/12 14:36:45, weiliang.lin2 wrote: > On 2013/08/12 06:24:15, weiliang.lin2 wrote: > > On 2013/08/11 ...
7 years, 4 months ago (2013-08-12 14:41:00 UTC) #14
Toon Verwaest
Lgtm, thanks. I'll land it when the tree is open again.
7 years, 4 months ago (2013-08-12 17:51:11 UTC) #15
Toon Verwaest
7 years, 3 months ago (2013-08-27 13:55:16 UTC) #16
Message was sent while issue was closed.
Committed patchset #6 manually as r16361 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698