Reland "[turbofan] Improve StoreStoreElimination"

This is a reland of 863bc2b8

Diff to original:
- Don't eliminate GC observable stores that were temporarily
  unobservable during traversal.
- Skip the previously added test for single-generation
- Add new test

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: v8:12200, chromium:1276923, v8:12477
Change-Id: Ied45ee28ac12b370f7b232d2d338f93e10fea6b4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3320460Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78349}

src/compiler/effect-control-linearizer.cc

@@ -342,6 +342,7 @@ 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_;
@@ -864,6 +865,7 @@ 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);
@@ -874,6 +876,7 @@ 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);
@@ -891,6 +894,14 @@ 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,6 +91,9 @@ 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;
 }
@@ -1882,7 +1885,6 @@ 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)      \
@@ -1906,6 +1908,17 @@ 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,6 +84,10 @@ 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),
@@ -92,18 +96,18 @@ struct FieldAccess {
         machine_type(MachineType::None()),
         write_barrier_kind(kFullWriteBarrier),
         const_field_info(ConstFieldInfo::None()),
-        is_store_in_literal(false) {}
+        is_store_in_literal(false),
+        maybe_initializing_or_transitioning_store(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),
@@ -112,12 +116,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(),
@@ -1043,7 +1047,8 @@ class V8_EXPORT_PRIVATE SimplifiedOperatorBuilder final
 
   const Operator* LoadFieldByIndex();
   const Operator* LoadField(FieldAccess const&);
-  const Operator* StoreField(FieldAccess const&);
+  const Operator* StoreField(FieldAccess const&,
+                             bool maybe_initializing_or_transitioning = true);
 
   // load-element [base + index]
   const Operator* LoadElement(ElementAccess const&);

src/compiler/store-store-elimination.cc

@@ -50,6 +50,7 @@ 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_);
@@ -76,13 +77,15 @@ size_t hash_value(const UnobservableStore& p) {
 // an UnobservablesSet allocates no memory.
 class UnobservablesSet final {
  private:
-  using KeyT = UnobservableStore;
-  using ValueT = bool;  // Emulates set semantics in the map.
+  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.
+  };
 
-  // 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;
+  using KeyT = UnobservableStore;
+  using ValueT = ObservableState;
 
  public:
   using SetT = PersistentMap<KeyT, ValueT>;
@@ -97,13 +100,17 @@ 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 in the set, we don't have to
+  // as parameter. If said obs it's already unobservable, we don't have to
   // create a new one.
   UnobservablesSet Add(UnobservableStore obs, Zone* zone) const;
 
@@ -113,18 +120,43 @@ 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 we currently we don't have the means to know if
+  // alias one another, and 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();
   }
-  bool Contains(UnobservableStore obs) const {
-    return set_ != nullptr && set_->Get(obs) != kNotPresent;
+
+  // 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 operator==(const UnobservablesSet& other) const {
@@ -145,22 +177,22 @@ class UnobservablesSet final {
   explicit UnobservablesSet(const SetT* set) : set_(set) {}
 
   static SetT* NewSet(Zone* zone) {
-    return zone->New<UnobservablesSet::SetT>(zone, kNotPresent);
+    return zone->New<UnobservablesSet::SetT>(zone, kObservable);
   }
 
-  static void SetAdd(SetT* set, const KeyT& key) { set->Set(key, kPresent); }
+  static void SetAdd(SetT* set, const KeyT& key) {
+    set->Set(key, kUnobservable);
+  }
   static void SetErase(SetT* set, const KeyT& key) {
-    set->Set(key, kNotPresent);
+    set->Set(key, kObservable);
+  }
+  static void SetGCObservable(SetT* set, const KeyT& key) {
+    set->Set(key, kGCObservable);
   }
 
   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()
@@ -286,7 +318,8 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
       StoreOffset offset = ToOffset(access);
 
       UnobservableStore observation = {stored_to->id(), offset};
-      bool is_not_observable = uses.Contains(observation);
+      const bool is_not_observable = uses.IsUnobservable(
+          observation, access.maybe_initializing_or_transitioning_store);
 
       if (is_not_observable) {
         TRACE("  #%d is StoreField[+%d,%s](#%d), unobservable", node->id(),
@@ -295,10 +328,29 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
         to_remove().insert(node);
         return uses;
       } else {
-        TRACE("  #%d is StoreField[+%d,%s](#%d), observable, recording in set",
+        const bool is_gc_observable =
+            access.maybe_initializing_or_transitioning_store &&
+            uses.IsGCObservable(observation);
+        // A GC observable store could have been unobservable in a previous
+        // visit. This is possible if the node that previously shadowed the
+        // initializing store is now unobservable, due to additional stores
+        // added to the unobservables set. Example:
+        //            StoreA --> StoreB (init)
+        //               ^
+        //               |
+        // PathX --> Allocate <-- StoreC <-- PathY
+        // When traversing PathX, StoreA will shadow StoreB and we will
+        // eliminate StoreB. When traversing PathY, StoreA will be shadowed by
+        // StoreC and we will eliminate StoreA, but StoreB is now observable by
+        // GC and should not be eliminated.
+        if (is_gc_observable) {
+          to_remove().erase(node);
+        }
+        TRACE(
+            "  #%d is StoreField[+%d,%s](#%d), observable%s, recording in set",
             node->id(), offset,
             MachineReprToString(access.machine_type.representation()),
-              stored_to->id());
+            stored_to->id(), is_gc_observable ? " by GC" : "");
         return uses.Add(observation, temp_zone());
       }
     }
@@ -316,6 +368,17 @@ 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(),
@@ -444,6 +507,16 @@ 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 {
@@ -451,9 +524,9 @@ UnobservablesSet UnobservablesSet::Intersect(const UnobservablesSet& other,
 
   UnobservablesSet::SetT* intersection = NewSet(zone);
   for (auto triple : set()->Zip(*other.set())) {
-    if (std::get<1>(triple) && std::get<2>(triple)) {
-      intersection->Set(std::get<0>(triple), kPresent);
-    }
+    ObservableState new_state =
+        Intersect(std::get<1>(triple), std::get<2>(triple));
+    intersection->Set(std::get<0>(triple), new_state);
   }
 
   return UnobservablesSet(intersection);
@@ -461,7 +534,7 @@ UnobservablesSet UnobservablesSet::Intersect(const UnobservablesSet& other,
 
 UnobservablesSet UnobservablesSet::Add(UnobservableStore obs,
                                        Zone* zone) const {
-  if (set()->Get(obs) != kNotPresent) return *this;
+  if (set()->Get(obs) == kUnobservable) return *this;
 
   UnobservablesSet::SetT* new_set = NewSet(zone);
   *new_set = *set();
@@ -484,6 +557,19 @@ 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);

test/mjsunit/mjsunit.status

@@ -1503,6 +1503,7 @@
   'compiler/test-literal-map-migration': [SKIP],
   'compiler/deopt-pretenure': [SKIP],
   'compiler/fast-api-sequences-x64': [SKIP],
+  'compiler/regress-store-store-elim': [SKIP],
 
   # TODO(v8:12031): Reimplement elements kinds transitions when concurrent
   # inlining.

test/mjsunit/regress/regress-crbug-1276923.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 --gc-interval=10
+
+// Base case where a GC observable store might be temporarily shadowed.
+function foo() {
+  let i = 0.1;
+  eval();
+  if (i) {
+    const c = {};
+    eval();
+  }
+}
+%PrepareFunctionForOptimization(foo);
+foo();
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();
+
+// Stress execution with GCs.
+function bar() {
+  for (let cnt = 0, i = 655; cnt < 50000 && i !== 1; cnt++, i = i / 3) {
+    i %= 2;
+    const c = { "b": 1, "a":1, "c": 1,  "d": 1 };
+    eval();
+  }
+}
+bar();