Reland^3 "[turbofan] Use feedback when reducing global loads/stores."

This is a reland of 2d2c137492 without
changes. Offending chromium tests have been modified.

Original change's description:
> Reland^2 "[turbofan] Use feedback when reducing global loads/stores."
>
> This reverts commit ac85ab0a3d. 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 <neis@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#58393}

Change-Id: Ic6734201a6c45f2752488ab44b16859776802f51
Reviewed-on: https://chromium-review.googlesource.com/c/1408252
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58769}

src/compiler/js-native-context-specialization.cc

@@ -797,9 +797,6 @@ FieldAccess ForPropertyCellValue(MachineRepresentation representation,
 Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
     Node* node, Node* receiver, Node* value, Handle<Name> 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);
@@ -807,9 +804,25 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
   if (it.state() != LookupIterator::DATA) return NoChange();
   if (!it.GetHolder<JSObject>()->IsJSGlobalObject()) return NoChange();
   Handle<PropertyCell> 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> name,
+    AccessMode access_mode, Node* index, Handle<PropertyCell> property_cell) {
+  Node* effect = NodeProperties::GetEffectInput(node);
+  Node* control = NodeProperties::GetControlInput(node);
+
   Handle<Object> 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) {
@@ -986,58 +999,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).
+  LoadGlobalParameters const& p = LoadGlobalParametersOf(node->op());
-  base::Optional<ScriptContextTableRef::LookupResult> result =
+  if (!p.feedback().IsValid()) return NoChange();
-      native_context().script_context_table().lookup(name);
+  FeedbackNexus nexus(p.feedback().vector(), p.feedback().slot());
-  if (result) {
+
-    ObjectRef contents = result->context.get(result->index);
+  DCHECK(nexus.kind() == FeedbackSlotKind::kLoadGlobalInsideTypeof ||
-    if (contents.IsHeapObject() &&
+         nexus.kind() == FeedbackSlotKind::kLoadGlobalNotInsideTypeof);
-        contents.AsHeapObject().map().oddball_type() == OddballType::kHole) {
+  if (nexus.GetFeedback()->IsCleared()) return NoChange();
-      return NoChange();
+  Handle<Object> 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> 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,
+        javascript()->LoadContext(0, context_slot_index, immutable),
-        effect);
+        context_constant, effect);
     ReplaceWithValue(node, value, effect);
     return Replace(value);
   }
 
-  // Lookup the {name} on the global object instead.
+  CHECK(feedback->IsPropertyCell());
-  return ReduceGlobalAccess(node, nullptr, nullptr, name.object(),
+  // The wanted name belongs (or did belong) to a property on the global object
-                            AccessMode::kLoad);
+  // and the feedback is the cell holding its value.
+  return ReduceGlobalAccess(node, nullptr, nullptr, p.name(), AccessMode::kLoad,
+                            nullptr, Handle<PropertyCell>::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).
+  StoreGlobalParameters const& p = StoreGlobalParametersOf(node->op());
-  base::Optional<ScriptContextTableRef::LookupResult> result =
+  if (!p.feedback().IsValid()) return NoChange();
-      native_context().script_context_table().lookup(name);
+  FeedbackNexus nexus(p.feedback().vector(), p.feedback().slot());
-  if (result) {
+
-    ObjectRef contents = result->context.get(result->index);
+  DCHECK(nexus.kind() == FeedbackSlotKind::kStoreGlobalSloppy ||
-    if ((contents.IsHeapObject() &&
+         nexus.kind() == FeedbackSlotKind::kStoreGlobalStrict);
-         contents.AsHeapObject().map().oddball_type() == OddballType::kHole) ||
+  if (nexus.GetFeedback()->IsCleared()) return NoChange();
-        result->immutable) {
+  Handle<Object> feedback(nexus.GetFeedback()->GetHeapObjectOrSmi(), isolate());
-      return NoChange();
+
+  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> 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),
+    Node* context_constant = jsgraph()->Constant(context);
-                              value, context, effect, control);
+    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.
+  CHECK(feedback->IsPropertyCell());
-  return ReduceGlobalAccess(node, nullptr, value, name.object(),
+  // The wanted name belongs (or did belong) to a property on the global object
-                            AccessMode::kStore);
+  // and the feedback is the cell holding its value.
+  return ReduceGlobalAccess(node, nullptr, value, p.name(), AccessMode::kStore,
+                            nullptr, Handle<PropertyCell>::cast(feedback));
 }
 
 Reduction JSNativeContextSpecialization::ReduceNamedAccess(

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> name, AccessMode access_mode,
                                Node* index = nullptr);
+  Reduction ReduceGlobalAccess(Node* node, Node* receiver, Node* value,
+                               Handle<Name> name, AccessMode access_mode,
+                               Node* index, Handle<PropertyCell> property_cell);
 
   Reduction ReduceSoftDeoptimize(Node* node, DeoptimizeReason reason);
   Reduction ReduceJSToString(Node* node);

src/feedback-vector.cc

@@ -724,16 +724,23 @@ void FeedbackNexus::ConfigurePropertyCellMode(Handle<PropertyCell> 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();

src/feedback-vector.h

@@ -671,8 +671,8 @@ class FeedbackNexus final {
   // For Global Load and Store ICs.
   void ConfigurePropertyCellMode(Handle<PropertyCell> cell);
   // Returns false if given combination of indices is not allowed.
-  bool ConfigureLexicalVarMode(int script_context_index,
+  bool ConfigureLexicalVarMode(int script_context_index, int context_slot_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

src/ic/ic.cc

@@ -514,8 +514,9 @@ MaybeHandle<Object> LoadGlobalIC::Load(Handle<Name> name) {
 
       bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
       if (use_ic) {
-        if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index,
+        if (nexus()->ConfigureLexicalVarMode(
-                                             lookup_result.slot_index)) {
+                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.
@@ -1376,8 +1377,9 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
 
     bool use_ic = (state() != NO_FEEDBACK) && FLAG_use_ic;
     if (use_ic) {
-      if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index,
+      if (nexus()->ConfigureLexicalVarMode(
-                                           lookup_result.slot_index)) {
+              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.