Inline immutable property loads

Inline immutable property loads

When a non-configurable, non-writable field is
read from a constant holder, the load is
eliminated and replaced with the direct value
of the field

BUG=
R=verwaest@chromium.org

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

[Toon Verwaest, 2014-04-10 11:58:31]

https://codereview.chromium.org/232853002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/232853002/diff/1/src/hydrogen.cc#newcode5354
src/hydrogen.cc:5354: Handle<Object> value(isolate()->heap()->the_hole_value(),
isolate());
Why do you pre-declare value to the hole rather than just doing Handle<Object>
value(lookup.GetLazyValue(), isolate()) ?

[p.antonov, 2014-04-10 14:32:25]

No reason, it's a silly mistake :)

> The safest solution for now is probably to call BuildCallConstantFunction if
types->length() == 0, before checking NeedsWrappingFor. And less intrusive.

Having moved the code to BuildMonomorphicAccess and further means the VisitCall
assumption about types->length() > 0 will hold without any extra code because
types->length() > 0 is ensured if BuildMonomorphicAccess is called. Sorry for
the confusion but there is therefore no need to do anything about VisitCall and
the previous patch would have only been an optimization had it worked.

Btw, in BuildMonomorphicAccess, is it possible for the first check
(info->GetJSObjectFieldAccess) to "steal" a possible inlining of immutable
properties, since that is now checked way later in BuildLoadNamedField?

[p.antonov, 2014-04-10 14:38:55]

> Btw, in BuildMonomorphicAccess, is it possible for the first check
> (info->GetJSObjectFieldAccess) to "steal" a possible inlining of immutable
> properties, since that is now checked way later in BuildLoadNamedField?

Never mind, I see this activates only for things such as array.length.

[Toon Verwaest, 2014-04-10 15:29:34]

Yes you are right. Even though string lengths are read-only, it would be blocked
by the short-cut. Not sure if you should worry about this for now though... The
problem is that such fields are foreign accessors rather than real fields.

Crankshaft knows about the accessors, and knows how to inline the access; but
doesn't even know whether they are read-only.

I wouldn't worry about them though. It probably won't affect anything that we
care about.

For the other part: if you really need to pollute the entire system with
read-only, non-configurable checks; do you think we can get by with just letting
the map-check in? I presume it will be GVNd out of loops anyway. The more
important thing is that now you can inline constants that can get constant
folded. Does that sound alright? Can you even measure the slow-down from the
map-checks?

[p.antonov, 2014-04-10 15:45:04]

> For the other part: if you really need to pollute the entire system with
> read-only, non-configurable checks; do you think we can get by with just
letting
> the map-check in? I presume it will be GVNd out of loops anyway. The more
> important thing is that now you can inline constants that can get constant
> folded. Does that sound alright? Can you even measure the slow-down from the
> map-checks?

Enabling more constant folding is the biggest gain yes.

Removing the unnecessary checks would just be additional "polish" (and would
avoid loading anything from memory in certain cases) - however if there is no
facilities to cleanly remove previously added checks when arriving in
BuildLoadNamedField then I suppose it's not worth it.

[Toon Verwaest, 2014-04-10 16:34:20]

Especially since we already omit map-checks for "stable maps" (which Math eg
probably is), I don't think it's worth doing that right now.

[Toon Verwaest, 2014-04-10 16:37:31]

So yeah, it is. Which means the map check will be gone anyway.

d8> %DebugPrint(Math)
DebugPrint: 0x22258931: [JSObject]
 - map = 0x5740d879 [FAST_HOLEY_ELEMENTS]

(gdb) p ((v8::internal::Map*)0x5740d879)->is_stable()
$3 = true

[p.antonov, 2014-04-11 12:09:37]

https://codereview.chromium.org/232853002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/232853002/diff/1/src/hydrogen.cc#newcode5354
src/hydrogen.cc:5354: Handle<Object> value(isolate()->heap()->the_hole_value(),
isolate());
On 2014/04/10 11:58:31, Toon Verwaest wrote:
> Why do you pre-declare value to the hole rather than just doing Handle<Object>
> value(lookup.GetLazyValue(), isolate()) ?

Done.

[Toon Verwaest, 2014-04-11 12:50:39]

lgtm, thanks

[Toon Verwaest, 2014-04-11 13:07:16]

Committed patchset #2 manually as r20690 (presubmit successful).