Cleanup IC heuristics.

Cleanup IC heuristics.

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

[Toon Verwaest, 2013-05-02 10:48:22]

PTAL

[danno, 2013-05-02 11:36:06]

https://codereview.chromium.org/14611006/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode185
src/ic.cc:185: if (target->is_keyed_load_stub() ||
// Determine whether the failure is due to a name failure or a prototype chain
failure.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode219
src/ic.cc:219: if (target->is_load_stub() || target->is_keyed_load_stub()) {
// For loads, handlers are stored in addition to the ICs on the map. Remove
those, too.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode229
src/ic.cc:229: if (cache_holder != OWN_MAP) return false;
// If the IC is shared between multiple receivers (slow dictionary mode), then
the map cannot be deprecated and the stub invalidated.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode234
src/ic.cc:234: return old_map == map || old_map->is_deprecated();
// The stub is not in the cache. We've ruled out all other kinds of failure
except for proptotype chain changes, a deprecated map, or a map that's different
from the one that the stub expects. If the map hasn't changed assume it's a
prototype failure. Tread deprecated maps in the same way as prototype failures
(stay monomorphic if possible).

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode1003
src/ic.cc:1003: bool overwrote = false;
// If the name doesn't match, go generic. Otherwise, if one of the maps already
in the polymorphic ic is the receiver map, just overwrite the handler. This
handles both the prototype chain changes and deprecated maps without prematurely
going generic.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode1012
src/ic.cc:1012: }
Move this loop about the code above to get the map count right. Better yet, fold
the two loops together.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode1056
src/ic.cc:1056: if (receiver_maps.at(i)->is_deprecated()) continue;
Maybe defer this to Crankshaft? Don't throw away the deprecated maps, just
replace them with their promoted version. Whatever you decide, please comment
about the reasoning.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode1514
src/ic.cc:1514: if (!value->FitsRepresentation(target_details.representation()))
{
Comment this.

https://codereview.chromium.org/14611006/diff/1/src/ic.cc#newcode1518
src/ic.cc:1518: *state = MONOMORPHIC_PROTOTYPE_FAILURE;
Check for already deprecated maps? Deprecated maps shouldn't change the target
at all?

[Toon Verwaest, 2013-05-02 12:27:20]

Addressed comments, PTAL again.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode185
src/ic.cc:185: if (target->is_keyed_load_stub() ||
On 2013/05/02 11:36:06, danno wrote:
> // Determine whether the failure is due to a name failure or a prototype chain
> failure.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode219
src/ic.cc:219: if (target->is_load_stub() || target->is_keyed_load_stub()) {
On 2013/05/02 11:36:06, danno wrote:
> // For loads, handlers are stored in addition to the ICs on the map. Remove
> those, too.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode229
src/ic.cc:229: if (cache_holder != OWN_MAP) return false;
On 2013/05/02 11:36:06, danno wrote:
> // If the IC is shared between multiple receivers (slow dictionary mode), then
> the map cannot be deprecated and the stub invalidated.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode234
src/ic.cc:234: return old_map == map || old_map->is_deprecated();
On 2013/05/02 11:36:06, danno wrote:
> // The stub is not in the cache. We've ruled out all other kinds of failure
> except for proptotype chain changes, a deprecated map, or a map that's
different
> from the one that the stub expects. If the map hasn't changed assume it's a
> prototype failure. Tread deprecated maps in the same way as prototype failures
> (stay monomorphic if possible).

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode1003
src/ic.cc:1003: bool overwrote = false;
On 2013/05/02 11:36:06, danno wrote:
> // If the name doesn't match, go generic. Otherwise, if one of the maps
already
> in the polymorphic ic is the receiver map, just overwrite the handler. This
> handles both the prototype chain changes and deprecated maps without
prematurely
> going generic. 

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode1012
src/ic.cc:1012: }
On 2013/05/02 11:36:06, danno wrote:
> Move this loop about the code above to get the map count right. Better yet,
fold
> the two loops together.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode1056
src/ic.cc:1056: if (receiver_maps.at(i)->is_deprecated()) continue;
On 2013/05/02 11:36:06, danno wrote:
> Maybe defer this to Crankshaft? Don't throw away the deprecated maps, just
> replace them with their promoted version. Whatever you decide, please comment
> about the reasoning.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode1514
src/ic.cc:1514: if (!value->FitsRepresentation(target_details.representation()))
{
On 2013/05/02 11:36:06, danno wrote:
> Comment this.

Done.

https://chromiumcodereview.appspot.com/14611006/diff/1/src/ic.cc#newcode1518
src/ic.cc:1518: *state = MONOMORPHIC_PROTOTYPE_FAILURE;
On 2013/05/02 11:36:06, danno wrote:
> Check for already deprecated maps? Deprecated maps shouldn't change the target
> at all?

Done.

[danno, 2013-05-02 15:24:28]

lgtm with nit

https://codereview.chromium.org/14611006/diff/6001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14611006/diff/6001/src/ic.cc#newcode991
src/ic.cc:991: int overwrite_handler = -1;
nit: handler_to_overwrite?

[Toon Verwaest, 2013-05-02 15:42:30]

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