Skip to content

Commit 9bebebd

Browse files
jakobkummerowCommit bot
jakobkummerow
authored and
Commit bot
committed
[ic] Restore PROPERTY key tracking in keyed ICs
Non-vectorized KeyedLoadICs used to remember whether they had seen Names as keys; Crankshaft uses this information to avoid emitting elements accesses which would always deopt. This CL restores that functionality for vector ICs. BUG=chromium:594183 LOG=y [email protected] Review URL: https://codereview.chromium.org/1912593002 Cr-Commit-Position: refs/heads/master@{#35706}
1 parent 6f43e1f commit 9bebebd

6 files changed

Lines changed: 138 additions & 53 deletions

File tree

‎src/ic/ic.cc‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -501,13 +501,21 @@ static bool MigrateDeprecated(Handle<Object> object) {
501501
return true;
502502
}
503503

504-
505-
void IC::ConfigureVectorState(IC::State new_state) {
504+
void IC::ConfigureVectorState(IC::State new_state, Handle<Object> key) {
506505
DCHECK(UseVector());
507506
if (new_state == PREMONOMORPHIC) {
508507
nexus()->ConfigurePremonomorphic();
509508
} else if (new_state == MEGAMORPHIC) {
510-
nexus()->ConfigureMegamorphic();
509+
if (kind() == Code::LOAD_IC || kind() == Code::STORE_IC) {
510+
nexus()->ConfigureMegamorphic();
511+
} else if (kind() == Code::KEYED_LOAD_IC) {
512+
KeyedLoadICNexus* nexus = casted_nexus<KeyedLoadICNexus>();
513+
nexus->ConfigureMegamorphicKeyed(key->IsName() ? PROPERTY : ELEMENT);
514+
} else {
515+
DCHECK(kind() == Code::KEYED_STORE_IC);
516+
KeyedStoreICNexus* nexus = casted_nexus<KeyedStoreICNexus>();
517+
nexus->ConfigureMegamorphicKeyed(key->IsName() ? PROPERTY : ELEMENT);
518+
}
511519
} else {
512520
UNREACHABLE();
513521
}
@@ -590,7 +598,7 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
590598
// Rewrite to the generic keyed load stub.
591599
if (FLAG_use_ic) {
592600
DCHECK(UseVector());
593-
ConfigureVectorState(MEGAMORPHIC);
601+
ConfigureVectorState(MEGAMORPHIC, name);
594602
TRACE_GENERIC_IC(isolate(), "LoadIC", "name as array index");
595603
TRACE_IC("LoadIC", name);
596604
}
@@ -777,7 +785,7 @@ void IC::PatchCache(Handle<Name> name, Handle<Code> code) {
777785
CopyICToMegamorphicCache(name);
778786
}
779787
DCHECK(UseVector());
780-
ConfigureVectorState(MEGAMORPHIC);
788+
ConfigureVectorState(MEGAMORPHIC, name);
781789
// Fall through.
782790
case MEGAMORPHIC:
783791
UpdateMegamorphicCache(*receiver_map(), *name, *code);
@@ -875,7 +883,7 @@ void LoadIC::UpdateCaches(LookupIterator* lookup) {
875883
if (state() == UNINITIALIZED) {
876884
// This is the first time we execute this inline cache. Set the target to
877885
// the pre monomorphic stub to delay setting the monomorphic state.
878-
ConfigureVectorState(PREMONOMORPHIC);
886+
ConfigureVectorState(PREMONOMORPHIC, Handle<Object>());
879887
TRACE_IC("LoadIC", lookup->name());
880888
return;
881889
}
@@ -1250,7 +1258,7 @@ MaybeHandle<Object> KeyedLoadIC::Load(Handle<Object> object,
12501258
}
12511259

12521260
if (!is_vector_set()) {
1253-
ConfigureVectorState(MEGAMORPHIC);
1261+
ConfigureVectorState(MEGAMORPHIC, key);
12541262
TRACE_GENERIC_IC(isolate(), "KeyedLoadIC", "set generic");
12551263
}
12561264
TRACE_IC("LoadIC", key);
@@ -1342,7 +1350,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
13421350
// Rewrite to the generic keyed store stub.
13431351
if (FLAG_use_ic) {
13441352
DCHECK(UseVector());
1345-
ConfigureVectorState(MEGAMORPHIC);
1353+
ConfigureVectorState(MEGAMORPHIC, name);
13461354
TRACE_IC("StoreIC", name);
13471355
TRACE_GENERIC_IC(isolate(), "StoreIC", "name as array index");
13481356
}
@@ -1458,7 +1466,7 @@ void StoreIC::UpdateCaches(LookupIterator* lookup, Handle<Object> value,
14581466
if (state() == UNINITIALIZED) {
14591467
// This is the first time we execute this inline cache. Set the target to
14601468
// the pre monomorphic stub to delay setting the monomorphic state.
1461-
ConfigureVectorState(PREMONOMORPHIC);
1469+
ConfigureVectorState(PREMONOMORPHIC, Handle<Object>());
14621470
TRACE_IC("StoreIC", lookup->name());
14631471
return;
14641472
}
@@ -1878,7 +1886,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
18781886
JSReceiver::MAY_BE_STORE_FROM_KEYED),
18791887
Object);
18801888
if (!is_vector_set()) {
1881-
ConfigureVectorState(MEGAMORPHIC);
1889+
ConfigureVectorState(MEGAMORPHIC, key);
18821890
TRACE_GENERIC_IC(isolate(), "KeyedStoreIC",
18831891
"unhandled internalized string key");
18841892
TRACE_IC("StoreIC", key);
@@ -1951,7 +1959,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
19511959
}
19521960

19531961
if (!is_vector_set()) {
1954-
ConfigureVectorState(MEGAMORPHIC);
1962+
ConfigureVectorState(MEGAMORPHIC, key);
19551963
TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "set generic");
19561964
}
19571965
TRACE_IC("StoreIC", key);

‎src/ic/ic.h‎

Lines changed: 1 addition & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ class IC {
106106
}
107107

108108
// Configure for most states.
109-
void ConfigureVectorState(IC::State new_state);
109+
void ConfigureVectorState(IC::State new_state, Handle<Object> key);
110110
// Configure the vector for MONOMORPHIC.
111111
void ConfigureVectorState(Handle<Name> name, Handle<Map> map,
112112
Handle<Code> handler);
@@ -267,10 +267,6 @@ class CallIC : public IC {
267267

268268
class LoadIC : public IC {
269269
public:
270-
static ExtraICState ComputeExtraICState(TypeofMode typeof_mode) {
271-
return LoadICState(typeof_mode).GetExtraICState();
272-
}
273-
274270
TypeofMode typeof_mode() const {
275271
return LoadICState::GetTypeofMode(extra_ic_state());
276272
}
@@ -320,20 +316,6 @@ class LoadIC : public IC {
320316

321317
class KeyedLoadIC : public LoadIC {
322318
public:
323-
// ExtraICState bits (building on IC)
324-
class IcCheckTypeField
325-
: public BitField<IcCheckType, LoadICState::kNextBitFieldOffset, 1> {};
326-
327-
static ExtraICState ComputeExtraICState(TypeofMode typeof_mode,
328-
IcCheckType key_type) {
329-
return LoadICState(typeof_mode).GetExtraICState() |
330-
IcCheckTypeField::encode(key_type);
331-
}
332-
333-
static IcCheckType GetKeyType(ExtraICState extra_state) {
334-
return IcCheckTypeField::decode(extra_state);
335-
}
336-
337319
KeyedLoadIC(FrameDepth depth, Isolate* isolate,
338320
KeyedLoadICNexus* nexus = NULL)
339321
: LoadIC(depth, isolate, nexus) {
@@ -366,10 +348,6 @@ class KeyedLoadIC : public LoadIC {
366348

367349
class StoreIC : public IC {
368350
public:
369-
static ExtraICState ComputeExtraICState(LanguageMode flag) {
370-
return StoreICState(flag).GetExtraICState();
371-
}
372-
373351
StoreIC(FrameDepth depth, Isolate* isolate, FeedbackNexus* nexus = NULL)
374352
: IC(depth, isolate, nexus) {
375353
DCHECK(IsStoreStub());
@@ -424,22 +402,6 @@ enum KeyedStoreIncrementLength { kDontIncrementLength, kIncrementLength };
424402

425403
class KeyedStoreIC : public StoreIC {
426404
public:
427-
// ExtraICState bits (building on IC)
428-
// ExtraICState bits
429-
// When more language modes are added, these BitFields need to move too.
430-
STATIC_ASSERT(i::LANGUAGE_END == 3);
431-
class ExtraICStateKeyedAccessStoreMode
432-
: public BitField<KeyedAccessStoreMode, 3, 3> {}; // NOLINT
433-
434-
class IcCheckTypeField : public BitField<IcCheckType, 6, 1> {};
435-
436-
static ExtraICState ComputeExtraICState(LanguageMode flag,
437-
KeyedAccessStoreMode mode) {
438-
return StoreICState(flag).GetExtraICState() |
439-
ExtraICStateKeyedAccessStoreMode::encode(mode) |
440-
IcCheckTypeField::encode(ELEMENT);
441-
}
442-
443405
KeyedAccessStoreMode GetKeyedAccessStoreMode() {
444406
return casted_nexus<KeyedStoreICNexus>()->GetKeyedAccessStoreMode();
445407
}

‎src/type-feedback-vector.cc‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,13 +340,32 @@ void FeedbackNexus::ConfigurePremonomorphic() {
340340

341341

342342
void FeedbackNexus::ConfigureMegamorphic() {
343+
// Keyed ICs must use ConfigureMegamorphicKeyed.
344+
DCHECK_NE(FeedbackVectorSlotKind::KEYED_LOAD_IC, vector()->GetKind(slot()));
345+
DCHECK_NE(FeedbackVectorSlotKind::KEYED_STORE_IC, vector()->GetKind(slot()));
346+
343347
Isolate* isolate = GetIsolate();
344348
SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
345349
SKIP_WRITE_BARRIER);
346350
SetFeedbackExtra(*TypeFeedbackVector::UninitializedSentinel(isolate),
347351
SKIP_WRITE_BARRIER);
348352
}
349353

354+
void KeyedLoadICNexus::ConfigureMegamorphicKeyed(IcCheckType property_type) {
355+
Isolate* isolate = GetIsolate();
356+
SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
357+
SKIP_WRITE_BARRIER);
358+
SetFeedbackExtra(Smi::FromInt(static_cast<int>(property_type)),
359+
SKIP_WRITE_BARRIER);
360+
}
361+
362+
void KeyedStoreICNexus::ConfigureMegamorphicKeyed(IcCheckType property_type) {
363+
Isolate* isolate = GetIsolate();
364+
SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
365+
SKIP_WRITE_BARRIER);
366+
SetFeedbackExtra(Smi::FromInt(static_cast<int>(property_type)),
367+
SKIP_WRITE_BARRIER);
368+
}
350369

351370
InlineCacheState LoadICNexus::StateFromFeedback() const {
352371
Isolate* isolate = GetIsolate();
@@ -824,10 +843,20 @@ KeyedAccessStoreMode KeyedStoreICNexus::GetKeyedAccessStoreMode() const {
824843
return mode;
825844
}
826845

846+
IcCheckType KeyedLoadICNexus::GetKeyType() const {
847+
Object* feedback = GetFeedback();
848+
if (feedback == *TypeFeedbackVector::MegamorphicSentinel(GetIsolate())) {
849+
return static_cast<IcCheckType>(Smi::cast(GetFeedbackExtra())->value());
850+
}
851+
return IsPropertyNameFeedback(feedback) ? PROPERTY : ELEMENT;
852+
}
827853

828854
IcCheckType KeyedStoreICNexus::GetKeyType() const {
829-
// The structure of the vector slots tells us the type.
830-
return GetFeedback()->IsName() ? PROPERTY : ELEMENT;
855+
Object* feedback = GetFeedback();
856+
if (feedback == *TypeFeedbackVector::MegamorphicSentinel(GetIsolate())) {
857+
return static_cast<IcCheckType>(Smi::cast(GetFeedbackExtra())->value());
858+
}
859+
return IsPropertyNameFeedback(feedback) ? PROPERTY : ELEMENT;
831860
}
832861
} // namespace internal
833862
} // namespace v8

‎src/type-feedback-vector.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,9 @@ class KeyedLoadICNexus : public FeedbackNexus {
475475
void ConfigurePolymorphic(Handle<Name> name, MapHandleList* maps,
476476
CodeHandleList* handlers);
477477

478+
void ConfigureMegamorphicKeyed(IcCheckType property_type);
479+
480+
IcCheckType GetKeyType() const;
478481
InlineCacheState StateFromFeedback() const override;
479482
Name* FindFirstName() const override;
480483
};
@@ -531,6 +534,7 @@ class KeyedStoreICNexus : public FeedbackNexus {
531534
void ConfigurePolymorphic(MapHandleList* maps,
532535
MapHandleList* transitioned_maps,
533536
CodeHandleList* handlers);
537+
void ConfigureMegamorphicKeyed(IcCheckType property_type);
534538

535539
KeyedAccessStoreMode GetKeyedAccessStoreMode() const;
536540
IcCheckType GetKeyType() const;

‎src/type-info.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ void TypeFeedbackOracle::KeyedPropertyReceiverTypes(
297297
KeyedLoadICNexus nexus(feedback_vector_, slot);
298298
CollectReceiverTypes(&nexus, receiver_types);
299299
*is_string = HasOnlyStringMaps(receiver_types);
300-
*key_type = nexus.FindFirstName() != NULL ? PROPERTY : ELEMENT;
300+
*key_type = nexus.GetKeyType();
301301
}
302302
}
303303

‎test/mjsunit/regress/regress-crbug-594183.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
// Copyright 2016 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --allow-natives-syntax
6+
7+
var global = {}
8+
9+
var fish = [
10+
{'name': 'foo'},
11+
{'name': 'bar'},
12+
];
13+
14+
for (var i = 0; i < fish.length; i++) {
15+
global[fish[i].name] = 1;
16+
}
17+
18+
function load() {
19+
var sum = 0;
20+
for (var i = 0; i < fish.length; i++) {
21+
var name = fish[i].name;
22+
sum += global[name];
23+
}
24+
return sum;
25+
}
26+
27+
load();
28+
load();
29+
%OptimizeFunctionOnNextCall(load);
30+
load();
31+
assertOptimized(load);
32+
33+
function store() {
34+
for (var i = 0; i < fish.length; i++) {
35+
var name = fish[i].name;
36+
global[name] = 1;
37+
}
38+
}
39+
40+
store();
41+
store();
42+
%OptimizeFunctionOnNextCall(store);
43+
store();
44+
assertOptimized(store);
45+
46+
// Regression test for KeyedStoreIC bug: would use PROPERTY mode erroneously.
47+
48+
function store_element(obj, key) {
49+
obj[key] = 0;
50+
}
51+
52+
var o1 = new Array(3);
53+
var o2 = new Array(3);
54+
o2.o2 = "o2";
55+
var o3 = new Array(3);
56+
o3.o3 = "o3";
57+
var o4 = new Array(3);
58+
o4.o4 = "o4";
59+
var o5 = new Array(3);
60+
o5.o5 = "o5";
61+
// Make the KeyedStoreIC megamorphic.
62+
store_element(o1, 0); // Premonomorphic
63+
store_element(o1, 0); // Monomorphic
64+
store_element(o2, 0); // 2-way polymorphic.
65+
store_element(o3, 0); // 3-way polymorphic.
66+
store_element(o4, 0); // 4-way polymorphic.
67+
store_element(o5, 0); // Megamorphic.
68+
69+
function inferrable_store(key) {
70+
store_element(o5, key);
71+
}
72+
73+
inferrable_store(0);
74+
inferrable_store(0);
75+
%OptimizeFunctionOnNextCall(inferrable_store);
76+
inferrable_store(0);
77+
assertOptimized(inferrable_store);
78+
// If |inferrable_store| emitted a generic keyed store, it won't deopt upon
79+
// seeing a property name key. It should have inferred a receiver map and
80+
// emitted an elements store, however.
81+
inferrable_store("deopt");
82+
assertUnoptimized(inferrable_store);

0 commit comments

Comments
 (0)