Track computed literal properties.

Track computed literal properties.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14982

[Toon Verwaest, 2013-05-10 18:57:21]

PTAL. From now on I'll just add tests ;)

[danno, 2013-06-06 07:54:19]

https://codereview.chromium.org/14721009/diff/22001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2351: if (!instr->value()->range()->IsInSmiRange()) {
What about also checking !instr->value()->type().IsSmi()? Also, careful with the
SmiRange check. On x64, I don't know if there is a guarantee that we won't get
tagged values in the SmiRange that are boxed, the range is independent of the
representation.

https://codereview.chromium.org/14721009/diff/22001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2475: if (!instr->value()->range()->IsInSmiRange()) {
See comment above.

https://codereview.chromium.org/14721009/diff/22001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/objects-debug.cc#newc...
src/objects-debug.cc:317: if (value->IsUndefined()) continue;
Maybe using another sentinel is a better idea for uninitialized fields, one that
can't be programmatically assigned to a field in a valid JavaScript program
(maybe the hole?).

https://codereview.chromium.org/14721009/diff/22001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/objects.cc#newcode1845
src/objects.cc:1845: if (value_type != REAL_VALUE) {
This clause seems like it could be integrated more cleanly with the above
"representation =", i.e. IsJSContextExtensionObject -> FORCE_TAGGED.

https://codereview.chromium.org/14721009/diff/22001/src/objects.cc#newcode2583
src/objects.cc:2583: !new_representation.IsDouble()) {
Why is double excluded from new_representation?

https://codereview.chromium.org/14721009/diff/22001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14721009/diff/22001/src/objects.h#newcode1602
src/objects.h:1602: enum ValueType {
A comment about the enum and the purpose of its values would be very helpful
here.

https://codereview.chromium.org/14721009/diff/22001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/parser.cc#newcode4002
src/parser.cc:4002: (value->IsNumber() || value->IsUndefined())) {
Might want to extend the comment above, i.e. doubles and unititalized fields.
BTW, is this sufficient? Don't you have to also check the representation of the
property in this case.

https://codereview.chromium.org/14721009/diff/22001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://codereview.chromium.org/14721009/diff/22001/src/x64/lithium-x64.cc#ne...
src/x64/lithium-x64.cc:2290: if (FLAG_track_fields &&
instr->field_representation().IsSmi()) {
See comment from ia32 code.

[Toon Verwaest, 2013-06-06 10:31:41]

Addressed comments. PTAL again.

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/arm/lithium-ar...
File src/arm/lithium-arm.cc (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/arm/lithium-ar...
src/arm/lithium-arm.cc:2351: if (!instr->value()->range()->IsInSmiRange()) {
This is an obsolete requirement. Nowadays we require the value to come in as a
smi, so it'll always be "InSmiRange()" and smi. Removing.

On 2013/06/06 07:54:19, danno wrote:
> What about also checking !instr->value()->type().IsSmi()? Also, careful with
the
> SmiRange check. On x64, I don't know if there is a guarantee that we won't get
> tagged values in the SmiRange that are boxed, the range is independent of the
> representation.

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects-debug....
src/objects-debug.cc:317: if (value->IsUndefined()) continue;
The hole value has a special meaning when placed in fields. I tried this before
and it is pretty messy. We could introduce a new dummy object for this purpose
if you feel strongly about it. Otherwise I'd leave it as it is.

On 2013/06/06 07:54:19, danno wrote:
> Maybe using another sentinel is a better idea for uninitialized fields, one
that
> can't be programmatically assigned to a field in a valid JavaScript program
> (maybe the hole?).

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects.cc#new...
src/objects.cc:1845: if (value_type != REAL_VALUE) {
On 2013/06/06 07:54:19, danno wrote:
> This clause seems like it could be integrated more cleanly with the above
> "representation =", i.e. IsJSContextExtensionObject -> FORCE_TAGGED.

Done.

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects.cc#new...
src/objects.cc:2583: !new_representation.IsDouble()) {
If we transition to double by simply setting the value, the boilerplate becomes
incompatible with its map, given that the boilerplate never sees such a change.
Hence we force generating new maps and overwriting the transitions. The
boilerplate will automatically transition later once it sees its own map is
deprecated.

On 2013/06/06 07:54:19, danno wrote:
> Why is double excluded from new_representation?

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/objects.h#newc...
src/objects.h:1602: enum ValueType {
On 2013/06/06 07:54:19, danno wrote:
> A comment about the enum and the purpose of its values would be very helpful
> here.

Done.

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/parser.cc
File src/parser.cc (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/parser.cc#newc...
src/parser.cc:4002: (value->IsNumber() || value->IsUndefined())) {
On 2013/06/06 07:54:19, danno wrote:
> Might want to extend the comment above, i.e. doubles and unititalized fields.
> BTW, is this sufficient? Don't you have to also check the representation of
the
> property in this case.

Done.

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/x64/lithium-x6...
File src/x64/lithium-x64.cc (right):

https://chromiumcodereview.appspot.com/14721009/diff/22001/src/x64/lithium-x6...
src/x64/lithium-x64.cc:2290: if (FLAG_track_fields &&
instr->field_representation().IsSmi()) {
On 2013/06/06 07:54:19, danno wrote:
> See comment from ia32 code.

Done.

[danno, 2013-06-06 13:07:32]

https://codereview.chromium.org/14721009/diff/32001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2354: }
Please split into another CL

https://codereview.chromium.org/14721009/diff/32001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2476: if (!instr->value()->type().IsHeapObject()) {
And this one, too.

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode2576
src/objects.cc:2576: if (old_representation.IsNone() &&
Add comment:
"It's fine to transition from None to anything but double without any
modification to the object, because the default undefined value for
representation None can be overwritten by both smi and tagged values. Doubles,
however, would require a box allocation."

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode4020
src/objects.cc:4020: : value->OptimalRepresentation();
Just pass value_type into OptimalRepresentation()

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode4067
src/objects.cc:4067: : value->OptimalRepresentation();
Same here.

https://codereview.chromium.org/14721009/diff/32001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/parser.cc#newcode4005
src/parser.cc:4005: (value->IsNumber() || value->IsUndefined())) {
As discussed, super scary.

[Toon Verwaest, 2013-06-06 13:22:26]

Addressed comments, ptal

https://codereview.chromium.org/14721009/diff/32001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2354: }
On 2013/06/06 13:07:32, danno wrote:
> Please split into another CL

Done.

https://codereview.chromium.org/14721009/diff/32001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2476: if (!instr->value()->type().IsHeapObject()) {
On 2013/06/06 13:07:32, danno wrote:
> And this one, too.

Done.

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode2576
src/objects.cc:2576: if (old_representation.IsNone() &&
On 2013/06/06 13:07:32, danno wrote:
> Add comment:
> "It's fine to transition from None to anything but double without any
> modification to the object, because the default undefined value for
> representation None can be overwritten by both smi and tagged values. Doubles,
> however, would require a box allocation." 

Done.

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode4020
src/objects.cc:4020: : value->OptimalRepresentation();
On 2013/06/06 13:07:32, danno wrote:
> Just pass value_type into OptimalRepresentation()

Done.

https://codereview.chromium.org/14721009/diff/32001/src/objects.cc#newcode4067
src/objects.cc:4067: : value->OptimalRepresentation();
On 2013/06/06 13:07:32, danno wrote:
> Same here.

Done.

https://codereview.chromium.org/14721009/diff/32001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/14721009/diff/32001/src/parser.cc#newcode4005
src/parser.cc:4005: (value->IsNumber() || value->IsUndefined())) {
On 2013/06/06 13:07:32, danno wrote:
> As discussed, super scary.

Done.

[danno, 2013-06-06 13:29:02]

https://codereview.chromium.org/14721009/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/14721009/diff/40001/src/objects-inl.h#newcode295
src/objects-inl.h:295: if (IsUndefined()) {
You missed one

https://codereview.chromium.org/14721009/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14721009/diff/40001/src/runtime.cc#newcode265
src/runtime.cc:265: } else if (FLAG_track_computed_fields &&
value->IsUndefined()) {
And here

[Toon Verwaest, 2013-06-06 13:32:35]

Updated those tests.

https://codereview.chromium.org/14721009/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/14721009/diff/40001/src/objects-inl.h#newcode295
src/objects-inl.h:295: if (IsUndefined()) {
On 2013/06/06 13:29:02, danno wrote:
> You missed one

Done.

https://codereview.chromium.org/14721009/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14721009/diff/40001/src/runtime.cc#newcode265
src/runtime.cc:265: } else if (FLAG_track_computed_fields &&
value->IsUndefined()) {
On 2013/06/06 13:29:02, danno wrote:
> And here

Done.

[Toon Verwaest, 2013-06-06 14:02:36]

Removed PLACEHOLDER_VALUE. PTAL again.

[danno, 2013-06-06 14:18:43]

lgtm

[Toon Verwaest, 2013-06-06 14:21:46]

Committed patchset #5 manually as r14982 (presubmit successful).