Issue 9167011: Support inlining and crankshaft optimization of Math.random.

Issue 9167011: Support inlining and crankshaft optimization of Math.random. (Closed)

Created:
8 years, 11 months ago by Mads Ager (chromium)

Modified:
8 years, 11 months ago

Reviewers:
fschneider, Jakob Kummerow

CC:
v8-dev, kisg, Paul Lind

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Support inlining and crankshaft optimization of Math.random. R=jkummerow@chromium.org BUG= TEST= Committed: http://code.google.com/p/v8/source/detail?r=10384

Patch Set 2 : Address review comments. #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+189 lines, -8 lines)			Patch
M	src/arm/lithium-arm.h	View		3 chunks	+13 lines, -1 line	0 comments	Download
M	src/arm/lithium-arm.cc	View	1	2 chunks	+10 lines, -1 line	0 comments	Download
M	src/arm/lithium-codegen-arm.cc	View	1	1 chunk	+24 lines, -0 lines	0 comments	Download
M	src/hydrogen.cc	View		2 chunks	+17 lines, -1 line	0 comments	Download
M	src/hydrogen-instructions.h	View	1	2 chunks	+18 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1	1 chunk	+23 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-ia32.h	View		3 chunks	+13 lines, -1 line	0 comments	Download
M	src/ia32/lithium-ia32.cc	View	1	2 chunks	+10 lines, -1 line	0 comments	Download
M	src/objects.h	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	src/x64/lithium-codegen-x64.cc	View	1	1 chunk	+32 lines, -0 lines	0 comments	Download
M	src/x64/lithium-x64.h	View		3 chunks	+13 lines, -1 line	0 comments	Download
M	src/x64/lithium-x64.cc	View	1	2 chunks	+14 lines, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Jakob Kummerow

LGTM with comments. http://codereview.chromium.org/9167011/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/9167011/diff/1/src/hydrogen-instructions.h#newcode3014 src/hydrogen-instructions.h:3014: : Representation::None(); "return Representation::Tagged();" should suffice, ...

8 years, 11 months ago (2012-01-11 08:10:40 UTC) #2

fschneider

dbc: http://codereview.chromium.org/9167011/diff/1/src/ia32/lithium-ia32.cc File src/ia32/lithium-ia32.cc (right): http://codereview.chromium.org/9167011/diff/1/src/ia32/lithium-ia32.cc#newcode1479 src/ia32/lithium-ia32.cc:1479: CAN_DEOPTIMIZE_EAGERLY); Since you don't have a DeoptimizeIf in ...

8 years, 11 months ago (2012-01-11 09:16:28 UTC) #3

Mads Ager (chromium)

8 years, 11 months ago (2012-01-11 10:24:45 UTC) #4

http://codereview.chromium.org/9167011/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/9167011/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:3014: : Representation::None();
On 2012/01/11 08:10:40, Jakob wrote:
> "return Representation::Tagged();" should suffice, as index == 0 is always
true.

Done.

http://codereview.chromium.org/9167011/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/9167011/diff/1/src/ia32/lithium-ia32.cc#newcod...
src/ia32/lithium-ia32.cc:1479: CAN_DEOPTIMIZE_EAGERLY);
On 2012/01/11 09:16:28, fschneider wrote:
> Since you don't have a DeoptimizeIf in the generated code, you  need
> CANNOT_DEOPTIMIZE_EAGERLY here (which is the default).

Good catch! Thanks Florian. Done.

http://codereview.chromium.org/9167011/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/9167011/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2938: Register global_object = rdx;
On 2012/01/11 08:10:40, Jakob wrote:
> Unused variable. Also, s/rdx/rcx/.

Done.

http://codereview.chromium.org/9167011/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2945: #ifdef _WIN64
On 2012/01/11 08:10:40, Jakob wrote:
> If you made use of the |global_object| variable defined above, you wouldn't
need
> this #ifdef.

Whoops. Thanks Jakob, that was indeed the intention.

Expand Messages | Collapse Messages