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

This reverts commit 8683116e.

Reason for revert: There's a bug in the StoreGlobalIC that causes regressions in combination with this CL (observable in Chrome but
not in d8).

Original change's description:
> Reland^3 "[turbofan] Use feedback when reducing global loads/stores."
>
> This is a reland of 2d2c1374 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 ac85ab0a. 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}

TBR=neis@chromium.org,sigurds@chromium.org,bmeurer@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:922545, chromium:922514, chromium:922558
Change-Id: I6e4c4c0fbc29a0f2a03972f1687242ae247ebfa8
Reviewed-on: https://chromium-review.googlesource.com/c/1417614
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58891}

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

@@ -797,6 +797,9 @@ 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);
@@ -804,25 +807,9 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
   if (it.state() != LookupIterator::DATA) return NoChange();
   if (!it.GetHolder<JSObject>()->IsJSGlobalObject()) return NoChange();
   Handle<PropertyCell> property_cell = it.GetPropertyCell();
-  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();
+  Handle<Object> property_cell_value(property_cell->value(), isolate());
   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) {
@@ -999,100 +986,58 @@ 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);
 
-  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<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())));
+  // Try to lookup the name on the script context table first (lexical scoping).
+  base::Optional<ScriptContextTableRef::LookupResult> 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();
     }
-
-    Node* context_constant = jsgraph()->Constant(context);
+    Node* context = jsgraph()->Constant(result->context);
     Node* value = effect = graph()->NewNode(
-        javascript()->LoadContext(0, context_slot_index, immutable),
-        context_constant, effect);
+        javascript()->LoadContext(0, result->index, result->immutable), context,
+        effect);
     ReplaceWithValue(node, value, effect);
     return Replace(value);
   }
 
-  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<PropertyCell>::cast(feedback));
+  // Lookup the {name} on the global object instead.
+  return ReduceGlobalAccess(node, nullptr, nullptr, name.object(),
+                            AccessMode::kLoad);
 }
 
 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);
 
-  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<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 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())));
+  // Try to lookup the name on the script context table first (lexical scoping).
+  base::Optional<ScriptContextTableRef::LookupResult> 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();
     }
-
-    Node* context_constant = jsgraph()->Constant(context);
-    effect = graph()->NewNode(javascript()->StoreContext(0, context_slot_index),
-                              value, context_constant, effect, control);
+    Node* context = jsgraph()->Constant(result->context);
+    effect = graph()->NewNode(javascript()->StoreContext(0, result->index),
+                              value, context, effect, control);
     ReplaceWithValue(node, value, effect, control);
     return Replace(value);
   }
 
-  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<PropertyCell>::cast(feedback));
+  // Lookup the {name} on the global object instead.
+  return ReduceGlobalAccess(node, nullptr, value, name.object(),
+                            AccessMode::kStore);
 }
 
 Reduction JSNativeContextSpecialization::ReduceNamedAccess(

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

@@ -112,9 +112,6 @@ 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

@@ -727,23 +727,16 @@ void FeedbackNexus::ConfigurePropertyCellMode(Handle<PropertyCell> cell) {
 }
 
 bool FeedbackNexus::ConfigureLexicalVarMode(int script_context_index,
-                                            int context_slot_index,
-                                            bool immutable) {
+                                            int context_slot_index) {
   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) ||
-      !ImmutabilityBit::is_valid(immutable)) {
+      !SlotIndexBits::is_valid(context_slot_index)) {
     return false;
   }
   int config = ContextIndexBits::encode(script_context_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);
+               SlotIndexBits::encode(context_slot_index);
 
   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, int context_slot_index,
-                               bool immutable);
+  bool ConfigureLexicalVarMode(int script_context_index,
+                               int context_slot_index);
   void ConfigureHandlerMode(const MaybeObjectHandle& handler);
 
   // For CloneObject ICs
@@ -682,8 +682,7 @@ 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, 18, _)    \
-  V(ImmutabilityBit, bool, 1, _)
+  V(SlotIndexBits, unsigned, 19, _)
 
   DEFINE_BIT_FIELDS(LEXICAL_MODE_BIT_FIELDS)
 #undef LEXICAL_MODE_BIT_FIELDS

src/ic/ic.cc

@@ -514,9 +514,8 @@ 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, lookup_result.slot_index,
-                lookup_result.mode == VariableMode::kConst)) {
+        if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index,
+                                             lookup_result.slot_index)) {
           TRACE_HANDLER_STATS(isolate(), LoadGlobalIC_LoadScriptContextField);
         } else {
           // Given combination of indices can't be encoded, so use slow stub.
@@ -1377,9 +1376,8 @@ 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, lookup_result.slot_index,
-              lookup_result.mode == VariableMode::kConst)) {
+      if (nexus()->ConfigureLexicalVarMode(lookup_result.context_index,
+                                           lookup_result.slot_index)) {
         TRACE_HANDLER_STATS(isolate(), StoreGlobalIC_StoreScriptContextField);
       } else {
         // Given combination of indices can't be encoded, so use slow stub.