Issue 6961019: Prevent deopt on double value assignment to typed arrays

Issue 6961019: Prevent deopt on double value assignment to typed arrays (Closed)

Created:
9 years, 7 months ago by danno

Modified:
9 years, 6 months ago

Reviewers:
Kevin Millikin (Chromium)

CC:
v8-dev

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

Visibility:
Public.

Description

Prevent deopt on double value assignment to typed arrays Implement truncation of double and tagged values when assigning to an element of a typed arrays in order to avoid depots. BUG=1313 TEST=test/mjsunit/external-array.js Committed: http://code.google.com/p/v8/source/detail?r=8077

Patch Set 1 #

Patch Set 2 : implement x64 #

Patch Set 3 : implement arm #

Patch Set 4 : merge with bleeding edge #

Patch Set 5 : Kevin's feedback #

Total comments: 8

Patch Set 6 : more review feedback #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+186 lines, -37 lines)			Patch
M	src/arm/lithium-arm.h	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
	src/arm/lithium-arm.cc	View	1 2 3 4 5	1 chunk	+25 lines, -0 lines	0 comments	Download
M	src/arm/stub-cache-arm.cc	View	1 2 3 4 5	1 chunk	+1 line, -17 lines	0 comments	Download
M	src/hydrogen.cc	View	1 2 3 4	1 chunk	+22 lines, -4 lines	0 comments	Download
M	src/hydrogen-instructions.h	View	1 2 3 4	4 chunks	+40 lines, -2 lines	0 comments	Download
M	src/ia32/lithium-ia32.h	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/ia32/lithium-ia32.cc	View	1 2 3 4 5	3 chunks	+33 lines, -3 lines	0 comments	Download
M	src/x64/lithium-x64.h	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/x64/lithium-x64.cc	View	1 2 3 4	2 chunks	+28 lines, -2 lines	0 comments	Download
M	test/mjsunit/external-array.js	View	1 2 3 4	5 chunks	+31 lines, -3 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

danno

Please take another look (I'll fix the pixel array clamping in another CL).

9 years, 7 months ago (2011-05-25 22:16:53 UTC) #2

Kevin Millikin (Chromium)

LGTM with superficial comments below. http://codereview.chromium.org/6961019/diff/11001/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/6961019/diff/11001/src/arm/lithium-arm.cc#newcode1775 src/arm/lithium-arm.cc:1775: LDoubleToI* res = new ...

9 years, 7 months ago (2011-05-26 07:53:37 UTC) #3

danno

9 years, 6 months ago (2011-06-01 13:14:41 UTC) #4

Done and landed.

http://codereview.chromium.org/6961019/diff/11001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/6961019/diff/11001/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1775: LDoubleToI* res = new LDoubleToI(reg,
On 2011/05/26 07:53:37, Kevin Millikin wrote:
> Two space indent here.
> 
> I'd name the two TempRegisters.  There is some dependence on the order uses
are
> communicated to the register allocator.  The use at this site is OK (right
now),
> but it would be better to have:
> 
> LOperand* temp1 = TempRegister();
> LOperand* temp2 = TempRegister();
> LDoubleToI* res = new LDoubleToI(reg, temp1, temp2);
> 
> if for no other reason than it is deterministic across compilers.
> 
> You can probably hoist temp1 and temp2 out of the if.

Done.

http://codereview.chromium.org/6961019/diff/11001/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1789: LInstruction* res = DefineSameAsFirst(
On 2011/05/26 07:53:37, Kevin Millikin wrote:
> I like
> 
> LTaggedToI* res = new LTaggedToI(reg, temp1, temp2, temp3);
> return AssigneEnvironment(DefineSameAsFirst(res));
> 
> better.  It's consistent with the first clause and avoids the line break :)

Done.

http://codereview.chromium.org/6961019/diff/11001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6961019/diff/11001/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3934: __ EmitECMATruncate(r5,
On 2011/05/26 07:53:37, Kevin Millikin wrote:
> Might as well put the args all on the same line.

Done.

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

http://codereview.chromium.org/6961019/diff/11001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1804: LOperand* xmm_temp =
On 2011/05/26 07:53:37, Kevin Millikin wrote:
> Some of this code can be lifted out of the if (the allocation of xmm_temp
> before, and possibly AssignEnvironment after depending on your taste.

Done.

Expand Messages | Collapse Messages