Remake of the load elimination fix made earlier (r18884).

Remake of the load elimination fix made earlier (r18884).

R=titzer@chromium.org, verwaest@chromium.org

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

[Igor Sheludko, 2014-01-30 13:03:58]

PTAL

[Igor Sheludko, 2014-01-31 14:09:40]

In patch set 2 I removed the map from the HObjectAccess and added an
"existing_inobject_property" flag instead.

[titzer, 2014-02-03 13:45:39]

LGTM other than naming.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6388: // The entry could be either previously
initialized or not.
Please preserve the comments here.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5912: static HObjectAccess
ForJSObjectOffset(Handle<Map> map, int offset,
Can we call this one ForMapAndOffset?

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5918: static HObjectAccess
ForJSObjectOffsetUnsafe(int offset,
And then we can keep the ForJSObjectOffset for this one.

[Toon Verwaest, 2014-02-03 13:51:32]

I find "Unsafe" a bad name for what it's doing. It's totally unclear from the
name.

I also think the "unsafe" variant is used way too frequently. We should always
use a map when there is one. I basically only see one valid use-case currently
for the unsafe variant: where we are clearing the next word after an object.
That value has nothing to do with the current object. And for that usecase, we
should definitely not track that value, since it's not part of any object. Which
is the opposite of what "unsafe" does now, afaict.

All other accesses should get a map as input. Most often you know exactly which
map is used. Only if there's a type-check involved we don't know the exact map.
In that case maybe we should use a prototypical map. Eg for JSValue, just use a
sample map of JSValues (eg the number map). For string map, use any of the
string maps as sample. They should always have the same instance_size anyway (as
indicated by the instance type).

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3598: Representation::Smi());
filler_map

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4248: if (!map.is_null()) {
I think you should use map->instance_size() - map->unused_property_fields() *
kPointerSize as the limit between visible and nonvisible memory; that way also
header-fields and hidden inobject properties are included.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6514: obj->IsAllocate() ||
obj->IsInnerAllocatedObject());
The equivalent ASSERT should still hold using the access'
ExistingInobjectPropertyField right?

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6704: obj->IsAllocate() ||
obj->IsInnerAllocatedObject());
Same as above

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5756: return HObjectAccess(kDouble,
spurious change

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5939: static HObjectAccess ForJSTypedArrayLength() {
You can probably get a map for each of the unsafe accesses below.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8021
src/hydrogen.cc:8021:
HObjectAccess::ForJSObjectOffsetUnsafe(JSFunction::kContextOffset));
target->map();

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8028
src/hydrogen.cc:8028: GlobalObject::kGlobalReceiverOffset));
target->context()->global_object()->map()

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8418
src/hydrogen.cc:8418: JSFunction::kPrototypeOrInitialMapOffset));
constructor->map()

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8525
src/hydrogen.cc:8525: HObjectAccess::ForJSObjectOffsetUnsafe(offset),
I presume we can get the map in from where we start building this?

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10404:
HObjectAccess::ForJSObjectOffsetUnsafe(JSValue::kValueOffset)));
Maybe we should pass in a prototypical JSValue map? Eg the one for numbers?

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10475:
HObjectAccess::ForJSObjectOffsetUnsafe(JSValue::kValueOffset), value);
Same here.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.h#newcode2313
src/hydrogen.h:2313: *access = HObjectAccess::ForJSObjectOffsetUnsafe(offset);
Use map()

[Igor Sheludko, 2014-02-03 17:57:04]

Thank you for your notes. Here is another revision.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3598: Representation::Smi());
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> filler_map

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4248: if (!map.is_null()) {
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> I think you should use map->instance_size() - map->unused_property_fields() *
> kPointerSize as the limit between visible and nonvisible memory; that way also
> header-fields and hidden inobject properties are included.

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6388: // The entry could be either previously
initialized or not.
On 2014/02/03 13:45:39, titzer wrote:
> Please preserve the comments here.

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6514: obj->IsAllocate() ||
obj->IsInnerAllocatedObject());
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> The equivalent ASSERT should still hold using the access'
> ExistingInobjectPropertyField right?

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6704: obj->IsAllocate() ||
obj->IsInnerAllocatedObject());
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> Same as above

The HObjectAccess mechanism is not applicable to HStoreKeyed.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5756: return HObjectAccess(kDouble,
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> spurious change

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5912: static HObjectAccess
ForJSObjectOffset(Handle<Map> map, int offset,
On 2014/02/03 13:45:39, titzer wrote:
> Can we call this one ForMapAndOffset?

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5918: static HObjectAccess
ForJSObjectOffsetUnsafe(int offset,
On 2014/02/03 13:45:39, titzer wrote:
> And then we can keep the ForJSObjectOffset for this one.

In offline discussions we have chosen a better name:
ForObservableJSObjectPropertyAt.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5939: static HObjectAccess ForJSTypedArrayLength() {
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> You can probably get a map for each of the unsafe accesses below.

For all these special cases we already know that the access is made to existing
in-object property. I would prefer not to pass maps in the cases below.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8021
src/hydrogen.cc:8021:
HObjectAccess::ForJSObjectOffsetUnsafe(JSFunction::kContextOffset));
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> target->map();

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8028
src/hydrogen.cc:8028: GlobalObject::kGlobalReceiverOffset));
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> target->context()->global_object()->map()

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8418
src/hydrogen.cc:8418: JSFunction::kPrototypeOrInitialMapOffset));
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> constructor->map()

Done.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode8525
src/hydrogen.cc:8525: HObjectAccess::ForJSObjectOffsetUnsafe(offset),
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> I presume we can get the map in from where we start building this?

We can't get the exact map for this access.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10404:
HObjectAccess::ForJSObjectOffsetUnsafe(JSValue::kValueOffset)));
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> Maybe we should pass in a prototypical JSValue map? Eg the one for numbers?

We can't get the exact map for this access.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10475:
HObjectAccess::ForJSObjectOffsetUnsafe(JSValue::kValueOffset), value);
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> Same here.

We can't get the exact map for this access.

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/149063010/diff/20001/src/hydrogen.h#newcode2313
src/hydrogen.h:2313: *access = HObjectAccess::ForJSObjectOffsetUnsafe(offset);
On 2014/02/03 13:51:32, Toon Verwaest wrote:
> Use map()

Done.

[Toon Verwaest, 2014-02-03 18:08:28]

lgtm with nits

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1201: Add<HStoreNamedField>(reference,
HObjectAccess::ForCounter(),
join lines

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1339: Add<HStoreNamedField>(object,
HObjectAccess::ForArrayLength(kind),
join lines

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2340: Add<HStoreNamedField>(elements,
HObjectAccess::ForFixedArrayLength(),
join lines

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2477: Add<HStoreNamedField>(object,
HObjectAccess::ForElementsPointer(),
join lines

[Igor Sheludko, 2014-02-03 19:17:57]

After rebasing and very final review of changes I found that the check in load
elimination was not fully correct. I also added an ASSERT that there must be no
loads from non observable in-object fields (see patch set 6).

PTAL.

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1201: Add<HStoreNamedField>(reference,
HObjectAccess::ForCounter(),
On 2014/02/03 18:08:28, Toon Verwaest wrote:
> join lines

Done.

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1339: Add<HStoreNamedField>(object,
HObjectAccess::ForArrayLength(kind),
On 2014/02/03 18:08:28, Toon Verwaest wrote:
> join lines

It will be too long line.

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2340: Add<HStoreNamedField>(elements,
HObjectAccess::ForFixedArrayLength(),
On 2014/02/03 18:08:28, Toon Verwaest wrote:
> join lines

Too long line.

https://codereview.chromium.org/149063010/diff/160001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2477: Add<HStoreNamedField>(object,
HObjectAccess::ForElementsPointer(),
On 2014/02/03 18:08:28, Toon Verwaest wrote:
> join lines

Too long line.

https://codereview.chromium.org/149063010/diff/340001/src/hydrogen-load-elimi...
File src/hydrogen-load-elimination.cc (right):

https://codereview.chromium.org/149063010/diff/340001/src/hydrogen-load-elimi...
src/hydrogen-load-elimination.cc:196: // There must be no loads from non
observable in-object properties.
New code.

https://codereview.chromium.org/149063010/diff/340001/src/hydrogen-load-elimi...
src/hydrogen-load-elimination.cc:224: if (instr->access().IsInobject() &&
New code.

[Toon Verwaest, 2014-02-04 10:13:49]

lgtm

[Igor Sheludko, 2014-02-04 10:48:58]

Committed patchset #7 manually as r19057 (presubmit successful).