Revert "[turbofan] Improve StoreStoreElimination"

This reverts commit 863bc2b8.

Reason for revert: https://crbug.com/1276923

Original change's description:
> [turbofan] Improve StoreStoreElimination
>
> Previously, StoreStoreElimination handled allocations as
> "can observe anything". This is pretty conservative and prohibits
> elimination of repeated double stores to the same field.
> With this CL allocations are changed to "observes initializing or
> transitioning stores".
> This way it is guaranteed that initializing stores to a freshly created
> object or stores that are part of a map transition are not eliminated
> before allocations (that can trigger GC), but allows elimination of
> non-initializing, non-transitioning, unobservable stores in the
> presence of allocations.
>
> Bug: v8:12200
> Change-Id: Ie1419696b9c8cb7c39aecf38d9f08102177b2c0f
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3295449
> Commit-Queue: Patrick Thier <pthier@chromium.org>
> Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
> Reviewed-by: Maya Lekova <mslekova@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#78230}

Bug: chromium:1276923
Change-Id: I43dc3572ce1ef1fda42b7551ce8210d9f03e36ca
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3318666
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78277}

src/compiler/effect-control-linearizer.cc

@@ -342,7 +342,6 @@ class EffectControlLinearizer {
   Zone* temp_zone_;
   MaintainSchedule maintain_schedule_;
   RegionObservability region_observability_ = RegionObservability::kObservable;
-  bool inside_region_ = false;
   SourcePositionTable* source_positions_;
   NodeOriginTable* node_origins_;
   JSHeapBroker* broker_;
@@ -865,7 +864,6 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state) {
   if (node->opcode() == IrOpcode::kFinishRegion) {
     // Reset the current region observability.
     region_observability_ = RegionObservability::kObservable;
-    inside_region_ = false;
     // Update the value uses to the value input of the finish node and
     // the effect uses to the effect input.
     return RemoveRenameNode(node);
@@ -876,7 +874,6 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state) {
     // StoreField and other operators).
     DCHECK_NE(RegionObservability::kNotObservable, region_observability_);
     region_observability_ = RegionObservabilityOf(node->op());
-    inside_region_ = true;
     // Update the value uses to the value input of the finish node and
     // the effect uses to the effect input.
     return RemoveRenameNode(node);
@@ -894,14 +891,6 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state) {
     return;
   }
 
-  if (node->opcode() == IrOpcode::kStoreField) {
-    // Mark stores outside a region as non-initializing and non-transitioning.
-    if (!inside_region_) {
-      const FieldAccess access = FieldAccessOf(node->op());
-      NodeProperties::ChangeOp(node, simplified()->StoreField(access, false));
-    }
-  }
-
   // The IfSuccess nodes should always start a basic block (and basic block
   // start nodes are not handled in the ProcessNode method).
   DCHECK_NE(IrOpcode::kIfSuccess, node->opcode());

src/compiler/simplified-operator.cc

@@ -91,9 +91,6 @@ std::ostream& operator<<(std::ostream& os, FieldAccess const& access) {
   if (access.is_store_in_literal) {
     os << " (store in literal)";
   }
-  if (access.maybe_initializing_or_transitioning_store) {
-    os << " (initializing or transitioning store)";
-  }
   os << "]";
   return os;
 }
@@ -1883,6 +1880,7 @@ const Operator* SimplifiedOperatorBuilder::SpeculativeNumberEqual(
 
 #define ACCESS_OP_LIST(V)                                                \
   V(LoadField, FieldAccess, Operator::kNoWrite, 1, 1, 1)                 \
+  V(StoreField, FieldAccess, Operator::kNoRead, 2, 1, 0)                 \
   V(LoadElement, ElementAccess, Operator::kNoWrite, 2, 1, 1)             \
   V(StoreElement, ElementAccess, Operator::kNoRead, 3, 1, 0)             \
   V(LoadTypedElement, ExternalArrayType, Operator::kNoWrite, 4, 1, 1)    \
@@ -1904,17 +1902,6 @@ const Operator* SimplifiedOperatorBuilder::SpeculativeNumberEqual(
 ACCESS_OP_LIST(ACCESS)
 #undef ACCESS
 
-const Operator* SimplifiedOperatorBuilder::StoreField(
-    const FieldAccess& access, bool maybe_initializing_or_transitioning) {
-  FieldAccess store_access = access;
-  store_access.maybe_initializing_or_transitioning_store =
-      maybe_initializing_or_transitioning;
-  return zone()->New<Operator1<FieldAccess>>(
-      IrOpcode::kStoreField,
-      Operator::kNoDeopt | Operator::kNoThrow | Operator::kNoRead, "StoreField",
-      2, 1, 1, 0, 1, 0, store_access);
-}
-
 const Operator* SimplifiedOperatorBuilder::LoadMessage() {
   return zone()->New<Operator>(IrOpcode::kLoadMessage, Operator::kEliminatable,
                                "LoadMessage", 1, 1, 1, 1, 1, 0);

src/compiler/simplified-operator.h

@@ -84,10 +84,6 @@ struct FieldAccess {
 #ifdef V8_HEAP_SANDBOX
   ExternalPointerTag external_pointer_tag = kExternalPointerNullTag;
 #endif
-  bool maybe_initializing_or_transitioning_store;  // store is potentially
-                                                   // initializing a newly
-                                                   // allocated object or part
-                                                   // of a map transition.
 
   FieldAccess()
       : base_is_tagged(kTaggedBase),
@@ -96,18 +92,18 @@ struct FieldAccess {
         machine_type(MachineType::None()),
         write_barrier_kind(kFullWriteBarrier),
         const_field_info(ConstFieldInfo::None()),
-        is_store_in_literal(false),
-        maybe_initializing_or_transitioning_store(false) {}
+        is_store_in_literal(false) {}
 
   FieldAccess(BaseTaggedness base_is_tagged, int offset, MaybeHandle<Name> name,
               MaybeHandle<Map> map, Type type, MachineType machine_type,
               WriteBarrierKind write_barrier_kind,
               ConstFieldInfo const_field_info = ConstFieldInfo::None(),
-              bool is_store_in_literal = false,
+              bool is_store_in_literal = false
 #ifdef V8_HEAP_SANDBOX
-              ExternalPointerTag external_pointer_tag = kExternalPointerNullTag,
+              ,
+              ExternalPointerTag external_pointer_tag = kExternalPointerNullTag
 #endif
-              bool maybe_initializing_or_transitioning_store = false)
+              )
       : base_is_tagged(base_is_tagged),
         offset(offset),
         name(name),
@@ -116,12 +112,12 @@ struct FieldAccess {
         machine_type(machine_type),
         write_barrier_kind(write_barrier_kind),
         const_field_info(const_field_info),
-        is_store_in_literal(is_store_in_literal),
+        is_store_in_literal(is_store_in_literal)
 #ifdef V8_HEAP_SANDBOX
-        external_pointer_tag(external_pointer_tag),
+        ,
+        external_pointer_tag(external_pointer_tag)
 #endif
-        maybe_initializing_or_transitioning_store(
-            maybe_initializing_or_transitioning_store) {
+  {
     DCHECK_GE(offset, 0);
     DCHECK_IMPLIES(
         machine_type.IsMapWord(),
@@ -1047,8 +1043,7 @@ class V8_EXPORT_PRIVATE SimplifiedOperatorBuilder final
 
   const Operator* LoadFieldByIndex();
   const Operator* LoadField(FieldAccess const&);
-  const Operator* StoreField(FieldAccess const&,
-                             bool maybe_initializing_or_transitioning = true);
+  const Operator* StoreField(FieldAccess const&);
 
   // load-element [base + index]
   const Operator* LoadElement(ElementAccess const&);

src/compiler/store-store-elimination.cc

@@ -50,7 +50,6 @@ using StoreOffset = uint32_t;
 struct UnobservableStore {
   NodeId id_;
   StoreOffset offset_;
-  bool maybe_gc_observable_ = false;
 
   bool operator==(const UnobservableStore other) const {
     return (id_ == other.id_) && (offset_ == other.offset_);
@@ -77,15 +76,13 @@ size_t hash_value(const UnobservableStore& p) {
 // an UnobservablesSet allocates no memory.
 class UnobservablesSet final {
  private:
-  enum ObservableState {
-    kObservable = 0,    // We haven't seen a store to this offset before.
-    kUnobservable = 1,  // Stores to the same offset can be eliminated.
-    kGCObservable = 2   // Stores to the same offset can only be eliminated,
-                        // if they are not initializing or transitioning.
-  };
-
   using KeyT = UnobservableStore;
-  using ValueT = ObservableState;
+  using ValueT = bool;  // Emulates set semantics in the map.
+
+  // The PersistentMap uses a special value to signify 'not present'. We use
+  // a boolean value to emulate set semantics.
+  static constexpr ValueT kNotPresent = false;
+  static constexpr ValueT kPresent = true;
 
  public:
   using SetT = PersistentMap<KeyT, ValueT>;
@@ -100,17 +97,13 @@ class UnobservablesSet final {
   UnobservablesSet& operator=(const UnobservablesSet& other)
       V8_NOEXCEPT = default;
 
-  // Computes the intersection of two states.
-  ObservableState Intersect(const ObservableState state1,
-                            const ObservableState state2) const;
-
   // Computes the intersection of two UnobservablesSets. If one of the sets is
   // empty, will return empty.
   UnobservablesSet Intersect(const UnobservablesSet& other,
                              const UnobservablesSet& empty, Zone* zone) const;
 
   // Returns a set that it is the current one, plus the observation obs passed
-  // as parameter. If said obs it's already unobservable, we don't have to
+  // as parameter. If said obs it's already in the set, we don't have to
   // create a new one.
   UnobservablesSet Add(UnobservableStore obs, Zone* zone) const;
 
@@ -120,43 +113,18 @@ class UnobservablesSet final {
   // This can probably be done better if the observations are stored first by
   // offset and then by node.
   // We are removing all nodes with offset off since different nodes may
-  // alias one another, and currently we don't have the means to know if
+  // alias one another, and we currently we don't have the means to know if
   // two nodes are definitely the same value.
   UnobservablesSet RemoveSameOffset(StoreOffset off, Zone* zone) const;
 
-  // Returns a new set where all observations are marked as being observable
-  // by GC.
-  UnobservablesSet MarkGCObservable(Zone* zone) const;
-
   const SetT* set() const { return set_; }
 
   bool IsUnvisited() const { return set_ == nullptr; }
   bool IsEmpty() const {
     return set_ == nullptr || set_->begin() == set_->end();
   }
-
-  // We need to guarantee that objects are fully initialized and fields are in
-  // sync with their map when a GC is triggered (potentially by any allocation).
-  // Therefore initializing or transitioning stores are observable if they are
-  // observable by GC. All other stores are not relevant for correct GC
-  // behaviour and can be eliminated even if they are observable by GC.
-  bool IsUnobservable(UnobservableStore obs,
-                      bool maybe_initializing_or_transitioning) const {
-    if (set_ == nullptr) return false;
-    ObservableState state = set_->Get(obs);
-    switch (state) {
-      case kUnobservable:
-        return true;
-      case kObservable:
-        return false;
-      case kGCObservable:
-        return !maybe_initializing_or_transitioning;
-    }
-    UNREACHABLE();
-  }
-
-  bool IsGCObservable(UnobservableStore obs) const {
-    return set_ != nullptr && set_->Get(obs) == kGCObservable;
+  bool Contains(UnobservableStore obs) const {
+    return set_ != nullptr && set_->Get(obs) != kNotPresent;
   }
 
   bool operator==(const UnobservablesSet& other) const {
@@ -177,22 +145,22 @@ class UnobservablesSet final {
   explicit UnobservablesSet(const SetT* set) : set_(set) {}
 
   static SetT* NewSet(Zone* zone) {
-    return zone->New<UnobservablesSet::SetT>(zone, kObservable);
+    return zone->New<UnobservablesSet::SetT>(zone, kNotPresent);
   }
 
-  static void SetAdd(SetT* set, const KeyT& key) {
-    set->Set(key, kUnobservable);
-  }
+  static void SetAdd(SetT* set, const KeyT& key) { set->Set(key, kPresent); }
   static void SetErase(SetT* set, const KeyT& key) {
-    set->Set(key, kObservable);
-  }
-  static void SetGCObservable(SetT* set, const KeyT& key) {
-    set->Set(key, kGCObservable);
+    set->Set(key, kNotPresent);
   }
 
   const SetT* set_ = nullptr;
 };
 
+// These definitions are here in order to please the linker, which in debug mode
+// sometimes requires static constants to be defined in .cc files.
+constexpr UnobservablesSet::ValueT UnobservablesSet::kNotPresent;
+constexpr UnobservablesSet::ValueT UnobservablesSet::kPresent;
+
 class RedundantStoreFinder final {
  public:
   // Note that we Initialize unobservable_ with js_graph->graph->NodeCount()
@@ -318,8 +286,7 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
       StoreOffset offset = ToOffset(access);
 
       UnobservableStore observation = {stored_to->id(), offset};
-      const bool is_not_observable = uses.IsUnobservable(
-          observation, access.maybe_initializing_or_transitioning_store);
+      bool is_not_observable = uses.Contains(observation);
 
       if (is_not_observable) {
         TRACE("  #%d is StoreField[+%d,%s](#%d), unobservable", node->id(),
@@ -328,15 +295,10 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
         to_remove().insert(node);
         return uses;
       } else {
-        TRACE(
-            "  #%d is StoreField[+%d,%s](#%d), observable%s, recording in set",
-            node->id(), offset,
-            MachineReprToString(access.machine_type.representation()),
-            stored_to->id(),
-            (access.maybe_initializing_or_transitioning_store &&
-             uses.IsGCObservable(observation))
-                ? " by GC"
-                : "");
+        TRACE("  #%d is StoreField[+%d,%s](#%d), observable, recording in set",
+              node->id(), offset,
+              MachineReprToString(access.machine_type.representation()),
+              stored_to->id());
         return uses.Add(observation, temp_zone());
       }
     }
@@ -354,17 +316,6 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
 
       return uses.RemoveSameOffset(offset, temp_zone());
     }
-    case IrOpcode::kAllocate:
-    case IrOpcode::kAllocateRaw: {
-      // Allocations can trigger a GC, therefore stores observable by allocation
-      // can not be eliminated, if they are initializing or tranisitioning
-      // stores.
-      TRACE(
-          "  #%d is Allocate or AllocateRaw, marking recorded offsets as "
-          "observable by GC",
-          node->id());
-      return uses.MarkGCObservable(temp_zone());
-    }
     default:
       if (CannotObserveStoreField(node)) {
         TRACE("  #%d:%s can observe nothing, set stays unchanged", node->id(),
@@ -493,16 +444,6 @@ UnobservablesSet UnobservablesSet::VisitedEmpty(Zone* zone) {
   return UnobservablesSet(NewSet(zone));
 }
 
-UnobservablesSet::ObservableState UnobservablesSet::Intersect(
-    const ObservableState state1, const ObservableState state2) const {
-  if (state1 == state2) return state1;
-  if (state1 == kObservable || state2 == kObservable) return kObservable;
-  if (state1 == kGCObservable || state2 == kGCObservable) {
-    return kGCObservable;
-  }
-  UNREACHABLE();
-}
-
 UnobservablesSet UnobservablesSet::Intersect(const UnobservablesSet& other,
                                              const UnobservablesSet& empty,
                                              Zone* zone) const {
@@ -510,9 +451,9 @@ UnobservablesSet UnobservablesSet::Intersect(const UnobservablesSet& other,
 
   UnobservablesSet::SetT* intersection = NewSet(zone);
   for (auto triple : set()->Zip(*other.set())) {
-    ObservableState new_state =
-        Intersect(std::get<1>(triple), std::get<2>(triple));
-    intersection->Set(std::get<0>(triple), new_state);
+    if (std::get<1>(triple) && std::get<2>(triple)) {
+      intersection->Set(std::get<0>(triple), kPresent);
+    }
   }
 
   return UnobservablesSet(intersection);
@@ -520,7 +461,7 @@ UnobservablesSet UnobservablesSet::Intersect(const UnobservablesSet& other,
 
 UnobservablesSet UnobservablesSet::Add(UnobservableStore obs,
                                        Zone* zone) const {
-  if (set()->Get(obs) == kUnobservable) return *this;
+  if (set()->Get(obs) != kNotPresent) return *this;
 
   UnobservablesSet::SetT* new_set = NewSet(zone);
   *new_set = *set();
@@ -543,19 +484,6 @@ UnobservablesSet UnobservablesSet::RemoveSameOffset(StoreOffset offset,
   return UnobservablesSet(new_set);
 }
 
-UnobservablesSet UnobservablesSet::MarkGCObservable(Zone* zone) const {
-  UnobservablesSet::SetT* new_set = NewSet(zone);
-  *new_set = *set();
-
-  // Mark all elements as observable by GC.
-  for (auto entry : *new_set) {
-    const UnobservableStore& obs = entry.first;
-    SetGCObservable(new_set, obs);
-  }
-
-  return UnobservablesSet(new_set);
-}
-
 }  // namespace
 
 // static

test/mjsunit/compiler/regress-store-store-elim.js

-// Copyright 2021 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Flags: --allow-natives-syntax --verify-heap --turbo-store-elimination
-
-// Check that transitioning stores are not eliminated.
-
-let obj = { a: 42 }
-
-function foo() {
-  // Force GC on the next allocation to trigger heap verification.
-  %SimulateNewspaceFull();
-
-  // Transitioning store. Must not be eliminated.
-  this.f = obj;
-
-  this.f = {
-    a: 43
-  };
-}
-
-%PrepareFunctionForOptimization(foo);
-var a;
-a = new foo();
-a = new foo();
-%OptimizeFunctionOnNextCall(foo);
-a = new foo();
-assertEquals(43, a.f.a);