Issue 15303004: Implement HChange support for Smis and use it in Load/StoreNameField

Issue 15303004: Implement HChange support for Smis and use it in Load/StoreNameField (Closed)

Created:
7 years, 7 months ago by danno

Modified:
7 years, 7 months ago

Reviewers:
Toon Verwaest, Rodolph Perfetta, Jakob Kummerow

CC:
v8-dev

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

Visibility:
Public.

More Reviews

Description

Implement HChange support for Smis and use it in Load/StoreNameField BUG= R=verwaest@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=14765

Patch Set 4 : Review feedback #

Total comments: 1

Created: 7 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+396 lines, -87 lines)			Patch
M	src/arm/lithium-arm.h	View	1 2	5 chunks	+35 lines, -1 line	0 comments	Download
M	src/arm/lithium-arm.cc	View	1 2	4 chunks	+26 lines, -1 line	0 comments	Download
M	src/arm/lithium-codegen-arm.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/arm/lithium-codegen-arm.cc	View	1 2 3	7 chunks	+59 lines, -11 lines	0 comments	Download
M	src/hydrogen.cc	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	src/hydrogen-instructions.h	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/hydrogen-instructions.cc	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1 2 3	7 chunks	+54 lines, -15 lines	0 comments	Download
M	src/ia32/lithium-ia32.h	View	1	5 chunks	+29 lines, -1 line	0 comments	Download
M	src/ia32/lithium-ia32.cc	View	1	5 chunks	+27 lines, -2 lines	1 comment	Download
M	src/lithium.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/lithium-allocator.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/property-details.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/x64/lithium-codegen-x64.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/x64/lithium-codegen-x64.cc	View	1 2 3	7 chunks	+60 lines, -11 lines	0 comments	Download
M	src/x64/lithium-x64.h	View	1	5 chunks	+29 lines, -1 line	0 comments	Download
M	src/x64/lithium-x64.cc	View	1	5 chunks	+29 lines, -3 lines	0 comments	Download
M	src/x64/macro-assembler-x64.cc	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A +	test/mjsunit/smi-representation.js	View		1 chunk	+32 lines, -32 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Rodolph Perfetta

drive by comments on arm https://codereview.chromium.org/15303004/diff/5001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/15303004/diff/5001/src/arm/lithium-codegen-arm.cc#newcode4712 src/arm/lithium-codegen-arm.cc:4712: __ SmiTag(ToRegister(output), ToRegister(input)); SetCC ...

7 years, 7 months ago (2013-05-18 17:54:53 UTC) #2

danno

Feedback addressed on ARM, also added -0 flag check on other platforms before adding -0 ...

7 years, 7 months ago (2013-05-18 19:16:15 UTC) #3

Toon Verwaest

lgtm (minus leftover in macro-assembler-x64.cc; which I removed). Landing.

7 years, 7 months ago (2013-05-23 08:31:27 UTC) #4

Toon Verwaest

Committed patchset #4 manually as r14765 (presubmit successful).

7 years, 7 months ago (2013-05-23 08:32:22 UTC) #5

Jakob Kummerow

7 years, 7 months ago (2013-05-23 12:49:31 UTC) #6

Message was sent while issue was closed.

https://chromiumcodereview.appspot.com/15303004/diff/9001/src/ia32/lithium-ia...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/15303004/diff/9001/src/ia32/lithium-ia...
src/ia32/lithium-ia32.cc:2053: return
AssignEnvironment(DefineSameAsFirst(new(zone()) LCheckSmi(value)));
This change causes unnecessary register pressure, because it makes the register
allocater think that LCheckSmi overwrites its input register.

Also, it is missing from the ARM port of this patch, making --trace-hydrogen
segfault on ARM.

I think we should introduce a separate LCheckSmiAndReturn instruction for the
use case in DoChange() above, and keep the original LCheckSmi unmodified (i.e.
not returning a value).

Expand Messages | Collapse Messages