Populate receiver types when there is no type feedback

Populate receiver types when there is no type feedback

When there is no type feedback yet, ComputeReceiverTypes
should still populate the SmallMapList when the receiver
is a HConstant.

BUG=
R=verwaest@chromium.org

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

[p.antonov, 2014-04-09 10:54:17]

This should fix the issue in VisitCall where the immutable
property inlining patch ends up returning an HConstant for a 
JSFunction and VisitCall assumes types->length() > 0 in this
situation.

I also have the immutable property inlining patch separately
implemented in BuildLoadNamedField already since it was required to trigger the
case in VisitCall.

[Toon Verwaest, 2014-04-09 14:08:16]

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

https://codereview.chromium.org/230363003/diff/1/src/hydrogen.cc#newcode5826
src/hydrogen.cc:5826: if (access_type != STORE || !map->is_observed()) {
So this case will still trigger for observed maps, right?

Can we replace !map->is_observed with CanInlinePropertyAccess(MapToType(map))
and add is_observed to the list of criteria there? That way the semantics for
PushLoad will be in sync with what we are checking for here, and we'll be
guaranteed that if PushLoad actually can inline a load (eg an HConstant), it'll
also be added by this code to the types-list for VisitCall.

[p.antonov, 2014-04-09 14:12:05]

Originally I planned to do that but speculated that the reason
observation is not checked in CanInlinePropertyAccess is
because it would punish loads unnecessarily (which should not
be affected by observation afaik?)

[p.antonov, 2014-04-09 14:16:19]

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

https://codereview.chromium.org/230363003/diff/1/src/hydrogen.cc#newcode5826
src/hydrogen.cc:5826: if (access_type != STORE || !map->is_observed()) {
On 2014/04/09 14:08:16, Toon Verwaest wrote:
> So this case will still trigger for observed maps, right?

When it is a load - yes (since loads should not be affected by 
observation). When it's a store and the map is observed,
adding the map to the typelist would cause some Object.observe
tests to fail.

[Toon Verwaest, 2014-04-09 14:40:05]

Ok yes, you are right. I can see this is correct.

But it would still be nice though to still move some of the logic into
CanInlinePropertyAccess, and pass in the access_type to that method. It would
filter out is_observed only if access_type == STORE.

I just want to ensure that we don't introduce such bugs later on, and have
everything correctly checked by CanInlinePropertyAccess.

[p.antonov, 2014-04-09 14:57:41]

Moved the check to CanInlinePropertyAccess

[Toon Verwaest, 2014-04-09 15:14:47]

Thanks! LGTM

[p.antonov, 2014-04-10 09:40:25]

So working on the check elimination patch, I think it will become overly
complicated if the checks are delayed. It would require BuildMonomorphicAccess
to have the immutable load logic sprinkled all over the place and necessisate
some repeated ugly checks as far as I can see. However if BuildLoadNamedField
simply walked the checked_object and removed the unnecessary checks from the
instruction stream after the fact (is this possible?), all the logic could
cleanly stay in BuildLoadNamedField.

The actual inlining patch right now is very clean with all code in
BuildLoadNamedField, however it isn't possible to stay that way if we have to
delay adding checks. I can put that patch up for review after you have commited
this patch.

[Toon Verwaest, 2014-04-10 09:50:00]

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

[Toon Verwaest, 2014-04-10 11:12:37]

So we had to revert this CL cause it breaks 

out/ia32.debug/d8 --test --random-seed=1773607309 --stress-opt --always-opt
--nohard-abort --nodead-code-elimination --nofold-constants
--enable-slow-asserts --debug-code --verify-heap --harmony-symbols
--harmony-collections --harmony-weak-collections --expose-gc
--allow-natives-syntax test/mjsunit/mjsunit.js test/mjsunit/harmony/symbols.js

I've looked at it for a moment, and haven't immediately figured out the exact
problem (apart from the fact that it's probably due to Realm in TestRegistry).

It seems like "making up new type feedback" like we are doing in this CL is
probably pretty fragile at this moment, given that we encode lots of knowledge
about what we can handle in the ICs, and expect Crankshaft to just follow the
ICs.

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

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

Could I do that in the same patch where the inlining happens
(https://codereview.chromium.org/232853002/) since it exposes the empty typelist
in the first place?

[Toon Verwaest, 2014-04-10 11:45:47]

Sure, that sounds fine. Sorry for the overhead, I should've seen this coming ...