From 2d2c1374929c7ff41bed6526103ba6217009057a Mon Sep 17 00:00:00 2001 From: Georg Neis Date: Thu, 20 Dec 2018 11:48:35 +0100 Subject: [PATCH] Reland^2 "[turbofan] Use feedback when reducing global loads/stores." This reverts commit ac85ab0a3d520a0c13b7e9224bf012cc4d9aa717. A chromium test caused trouble and was taken care of in https://chromium-review.googlesource.com/c/1384064. Original change's description: > [turbofan] Use feedback when reducing global loads/stores. > > We already record the script context location or the property cell > as feedback of the global load/store IC, so Turbofan doesn't need > to do the lookups again. TBR=sigurds@chromium.org Change-Id: I58bcd9bceec2f9cf401f7b0fc4460a6da6cd0abc Reviewed-on: https://chromium-review.googlesource.com/c/1386404 Commit-Queue: Georg Neis Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#58393} --- .../js-native-context-specialization.cc | 125 +++++++++++++----- .../js-native-context-specialization.h | 3 + src/feedback-vector.cc | 13 +- src/feedback-vector.h | 7 +- src/ic/ic.cc | 10 +- 5 files changed, 113 insertions(+), 45 deletions(-) diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc index 0e65b7dd92..1d12362324 100644 --- a/src/compiler/js-native-context-specialization.cc +++ b/src/compiler/js-native-context-specialization.cc @@ -795,9 +795,6 @@ FieldAccess ForPropertyCellValue(MachineRepresentation representation, Reduction JSNativeContextSpecialization::ReduceGlobalAccess( Node* node, Node* receiver, Node* value, Handle name, AccessMode access_mode, Node* index) { - Node* effect = NodeProperties::GetEffectInput(node); - Node* control = NodeProperties::GetControlInput(node); - // Lookup on the global object. We only deal with own data properties // of the global object here (represented as PropertyCell). LookupIterator it(isolate(), global_object(), name, LookupIterator::OWN); @@ -805,9 +802,25 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess( if (it.state() != LookupIterator::DATA) return NoChange(); if (!it.GetHolder()->IsJSGlobalObject()) return NoChange(); Handle property_cell = it.GetPropertyCell(); - PropertyDetails property_details = property_cell->property_details(); + return ReduceGlobalAccess(node, receiver, value, name, access_mode, index, + property_cell); +} + +Reduction JSNativeContextSpecialization::ReduceGlobalAccess( + Node* node, Node* receiver, Node* value, Handle name, + AccessMode access_mode, Node* index, Handle property_cell) { + Node* effect = NodeProperties::GetEffectInput(node); + Node* control = NodeProperties::GetControlInput(node); + Handle property_cell_value(property_cell->value(), isolate()); + if (property_cell_value.is_identical_to(factory()->the_hole_value())) { + // The property cell is no longer valid. + return NoChange(); + } + + PropertyDetails property_details = property_cell->property_details(); PropertyCellType property_cell_type = property_details.cell_type(); + DCHECK_EQ(kData, property_details.kind()); // We have additional constraints for stores. if (access_mode == AccessMode::kStore) { @@ -984,58 +997,100 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess( Reduction JSNativeContextSpecialization::ReduceJSLoadGlobal(Node* node) { DCHECK_EQ(IrOpcode::kJSLoadGlobal, node->opcode()); - NameRef name(broker(), LoadGlobalParametersOf(node->op()).name()); Node* effect = NodeProperties::GetEffectInput(node); - // Try to lookup the name on the script context table first (lexical scoping). - base::Optional result = - native_context().script_context_table().lookup(name); - if (result) { - ObjectRef contents = result->context.get(result->index); - if (contents.IsHeapObject() && - contents.AsHeapObject().map().oddball_type() == OddballType::kHole) { - return NoChange(); + LoadGlobalParameters const& p = LoadGlobalParametersOf(node->op()); + if (!p.feedback().IsValid()) return NoChange(); + FeedbackNexus nexus(p.feedback().vector(), p.feedback().slot()); + + DCHECK(nexus.kind() == FeedbackSlotKind::kLoadGlobalInsideTypeof || + nexus.kind() == FeedbackSlotKind::kLoadGlobalNotInsideTypeof); + if (nexus.GetFeedback()->IsCleared()) return NoChange(); + Handle feedback(nexus.GetFeedback()->GetHeapObjectOrSmi(), isolate()); + + if (feedback->IsSmi()) { + // The wanted name belongs to a script-scope variable and the feedback tells + // us where to find its value. + + int number = feedback->Number(); + int const script_context_index = + FeedbackNexus::ContextIndexBits::decode(number); + int const context_slot_index = FeedbackNexus::SlotIndexBits::decode(number); + bool const immutable = FeedbackNexus::ImmutabilityBit::decode(number); + Handle context = ScriptContextTable::GetContext( + isolate(), native_context().script_context_table().object(), + script_context_index); + + { + ObjectRef contents(broker(), + handle(context->get(context_slot_index), isolate())); + CHECK(!contents.equals(ObjectRef(broker(), factory()->the_hole_value()))); } - Node* context = jsgraph()->Constant(result->context); + + Node* context_constant = jsgraph()->Constant(context); Node* value = effect = graph()->NewNode( - javascript()->LoadContext(0, result->index, result->immutable), context, - effect); + javascript()->LoadContext(0, context_slot_index, immutable), + context_constant, effect); ReplaceWithValue(node, value, effect); return Replace(value); } - // Lookup the {name} on the global object instead. - return ReduceGlobalAccess(node, nullptr, nullptr, name.object(), - AccessMode::kLoad); + CHECK(feedback->IsPropertyCell()); + // The wanted name belongs (or did belong) to a property on the global object + // and the feedback is the cell holding its value. + return ReduceGlobalAccess(node, nullptr, nullptr, p.name(), AccessMode::kLoad, + nullptr, Handle::cast(feedback)); } Reduction JSNativeContextSpecialization::ReduceJSStoreGlobal(Node* node) { DCHECK_EQ(IrOpcode::kJSStoreGlobal, node->opcode()); - NameRef name(broker(), StoreGlobalParametersOf(node->op()).name()); Node* value = NodeProperties::GetValueInput(node, 0); Node* effect = NodeProperties::GetEffectInput(node); Node* control = NodeProperties::GetControlInput(node); - // Try to lookup the name on the script context table first (lexical scoping). - base::Optional result = - native_context().script_context_table().lookup(name); - if (result) { - ObjectRef contents = result->context.get(result->index); - if ((contents.IsHeapObject() && - contents.AsHeapObject().map().oddball_type() == OddballType::kHole) || - result->immutable) { - return NoChange(); + StoreGlobalParameters const& p = StoreGlobalParametersOf(node->op()); + if (!p.feedback().IsValid()) return NoChange(); + FeedbackNexus nexus(p.feedback().vector(), p.feedback().slot()); + + DCHECK(nexus.kind() == FeedbackSlotKind::kStoreGlobalSloppy || + nexus.kind() == FeedbackSlotKind::kStoreGlobalStrict); + if (nexus.GetFeedback()->IsCleared()) return NoChange(); + Handle feedback(nexus.GetFeedback()->GetHeapObjectOrSmi(), isolate()); + + if (feedback->IsSmi()) { + // The wanted name belongs to a script-scope variable and the feedback tells + // us where to find its value. + + int const script_context_index = + FeedbackNexus::ContextIndexBits::decode(feedback->Number()); + int const context_slot_index = + FeedbackNexus::SlotIndexBits::decode(feedback->Number()); + bool const immutable = + FeedbackNexus::ImmutabilityBit::decode(feedback->Number()); + Handle context = ScriptContextTable::GetContext( + isolate(), native_context().script_context_table().object(), + script_context_index); + + if (immutable) return NoChange(); + + { + ObjectRef contents(broker(), + handle(context->get(context_slot_index), isolate())); + CHECK(!contents.equals(ObjectRef(broker(), factory()->the_hole_value()))); } - Node* context = jsgraph()->Constant(result->context); - effect = graph()->NewNode(javascript()->StoreContext(0, result->index), - value, context, effect, control); + + Node* context_constant = jsgraph()->Constant(context); + effect = graph()->NewNode(javascript()->StoreContext(0, context_slot_index), + value, context_constant, effect, control); ReplaceWithValue(node, value, effect, control); return Replace(value); } - // Lookup the {name} on the global object instead. - return ReduceGlobalAccess(node, nullptr, value, name.object(), - AccessMode::kStore); + CHECK(feedback->IsPropertyCell()); + // The wanted name belongs (or did belong) to a property on the global object + // and the feedback is the cell holding its value. + return ReduceGlobalAccess(node, nullptr, value, p.name(), AccessMode::kStore, + nullptr, Handle::cast(feedback)); } Reduction JSNativeContextSpecialization::ReduceNamedAccess( diff --git a/src/compiler/js-native-context-specialization.h b/src/compiler/js-native-context-specialization.h index 1d75d6f035..0f7b6e8988 100644 --- a/src/compiler/js-native-context-specialization.h +++ b/src/compiler/js-native-context-specialization.h @@ -112,6 +112,9 @@ class V8_EXPORT_PRIVATE JSNativeContextSpecialization final Reduction ReduceGlobalAccess(Node* node, Node* receiver, Node* value, Handle name, AccessMode access_mode, Node* index = nullptr); + Reduction ReduceGlobalAccess(Node* node, Node* receiver, Node* value, + Handle name, AccessMode access_mode, + Node* index, Handle property_cell); Reduction ReduceSoftDeoptimize(Node* node, DeoptimizeReason reason); Reduction ReduceJSToString(Node* node); diff --git a/src/feedback-vector.cc b/src/feedback-vector.cc index 0491cb41e2..ee36e6faed 100644 --- a/src/feedback-vector.cc +++ b/src/feedback-vector.cc @@ -724,16 +724,23 @@ void FeedbackNexus::ConfigurePropertyCellMode(Handle cell) { } bool FeedbackNexus::ConfigureLexicalVarMode(int script_context_index, - int context_slot_index) { + int context_slot_index, + bool immutable) { DCHECK(IsGlobalICKind(kind())); DCHECK_LE(0, script_context_index); DCHECK_LE(0, context_slot_index); if (!ContextIndexBits::is_valid(script_context_index) || - !SlotIndexBits::is_valid(context_slot_index)) { + !SlotIndexBits::is_valid(context_slot_index) || + !ImmutabilityBit::is_valid(immutable)) { return false; } int config = ContextIndexBits::encode(script_context_index) | - SlotIndexBits::encode(context_slot_index); + SlotIndexBits::encode(context_slot_index) | + ImmutabilityBit::encode(immutable); + + // Force {config} to be in Smi range by propagating the most significant Smi + // bit. This does not change any of the bitfield's bits. + config = (config << (32 - kSmiValueSize)) >> (32 - kSmiValueSize); SetFeedback(Smi::FromInt(config)); Isolate* isolate = GetIsolate(); diff --git a/src/feedback-vector.h b/src/feedback-vector.h index b14d8672df..6285c26708 100644 --- a/src/feedback-vector.h +++ b/src/feedback-vector.h @@ -671,8 +671,8 @@ class FeedbackNexus final { // For Global Load and Store ICs. void ConfigurePropertyCellMode(Handle cell); // Returns false if given combination of indices is not allowed. - bool ConfigureLexicalVarMode(int script_context_index, - int context_slot_index); + bool ConfigureLexicalVarMode(int script_context_index, int context_slot_index, + bool immutable); void ConfigureHandlerMode(const MaybeObjectHandle& handler); // For CloneObject ICs @@ -682,7 +682,8 @@ class FeedbackNexus final { // Bit positions in a smi that encodes lexical environment variable access. #define LEXICAL_MODE_BIT_FIELDS(V, _) \ V(ContextIndexBits, unsigned, 12, _) \ - V(SlotIndexBits, unsigned, 19, _) + V(SlotIndexBits, unsigned, 18, _) \ + V(ImmutabilityBit, bool, 1, _) DEFINE_BIT_FIELDS(LEXICAL_MODE_BIT_FIELDS) #undef LEXICAL_MODE_BIT_FIELDS diff --git a/src/ic/ic.cc b/src/ic/ic.cc index a1ba60ab96..447465ab78 100644 --- a/src/ic/ic.cc +++ b/src/ic/ic.cc @@ -512,8 +512,9 @@ MaybeHandle LoadGlobalIC::Load(Handle name) { bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic; if (use_ic) { - if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index, - lookup_result.slot_index)) { + if (nexus()->ConfigureLexicalVarMode( + lookup_result.context_index, lookup_result.slot_index, + lookup_result.mode == VariableMode::kConst)) { TRACE_HANDLER_STATS(isolate(), LoadGlobalIC_LoadScriptContextField); } else { // Given combination of indices can't be encoded, so use slow stub. @@ -1374,8 +1375,9 @@ MaybeHandle StoreGlobalIC::Store(Handle name, bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic; if (use_ic) { - if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index, - lookup_result.slot_index)) { + if (nexus()->ConfigureLexicalVarMode( + lookup_result.context_index, lookup_result.slot_index, + lookup_result.mode == VariableMode::kConst)) { TRACE_HANDLER_STATS(isolate(), StoreGlobalIC_StoreScriptContextField); } else { // Given combination of indices can't be encoded, so use slow stub.