Commit 7796d60e authored by Patrick Thier's avatar Patrick Thier Committed by V8 LUCI CQ

Revert "[turbofan] Handle Allocations in StoreStoreElimination"

This reverts commit d87e5f42.

Reason for revert: Causes issues by eliminating stores that can be observed by GC. Flagging stores as "initializing" needs better handling than what was done in this CL.

Original change's description:
> [turbofan] Handle Allocations in 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 stores".
> This way it is guaranteed that initializing stores to a freshly created
> object are not eliminated before allocations (that can trigger GC), but
> allows elimination of non-initializing, unobservable stores in the
> presence of allocations.
>
> Bug: v8:12200
> Change-Id: I5ef1ca8892a84a3b332e081e2fa6285d0eba9d46
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3211585
> Commit-Queue: Patrick Thier <pthier@chromium.org>
> Reviewed-by: Maya Lekova <mslekova@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#77299}

Bug: v8:12200
Change-Id: I0f18cbc3e848011f1a998b073b05b3bdbc4e1223
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3218158
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Patrick Thier <pthier@chromium.org>
Reviewed-by: 's avatarMaya Lekova <mslekova@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77342}
parent d806ca7b
...@@ -50,7 +50,6 @@ using StoreOffset = uint32_t; ...@@ -50,7 +50,6 @@ using StoreOffset = uint32_t;
struct UnobservableStore { struct UnobservableStore {
NodeId id_; NodeId id_;
StoreOffset offset_; StoreOffset offset_;
bool is_initializing_; // Store is initializing a newly allocated object.
bool operator==(const UnobservableStore other) const { bool operator==(const UnobservableStore other) const {
return (id_ == other.id_) && (offset_ == other.offset_); return (id_ == other.id_) && (offset_ == other.offset_);
...@@ -114,16 +113,10 @@ class UnobservablesSet final { ...@@ -114,16 +113,10 @@ class UnobservablesSet final {
// This can probably be done better if the observations are stored first by // This can probably be done better if the observations are stored first by
// offset and then by node. // offset and then by node.
// We are removing all nodes with offset off since different nodes may // 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. // two nodes are definitely the same value.
UnobservablesSet RemoveSameOffset(StoreOffset off, Zone* zone) const; UnobservablesSet RemoveSameOffset(StoreOffset off, Zone* zone) const;
// Returns a set that it is the current one, except for all of the
// observations of an initializing store. This is done by creating a new set
// and copying all observations of non-initializing stores.
// Initializing stores are stores to a freshly created object.
UnobservablesSet RemoveInitializingStores(Zone* zone) const;
const SetT* set() const { return set_; } const SetT* set() const { return set_; }
bool IsUnvisited() const { return set_ == nullptr; } bool IsUnvisited() const { return set_ == nullptr; }
...@@ -291,16 +284,13 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet( ...@@ -291,16 +284,13 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
Node* stored_to = node->InputAt(0); Node* stored_to = node->InputAt(0);
const FieldAccess& access = FieldAccessOf(node->op()); const FieldAccess& access = FieldAccessOf(node->op());
StoreOffset offset = ToOffset(access); StoreOffset offset = ToOffset(access);
const bool is_initializing = NodeProperties::IsFreshObject(stored_to);
UnobservableStore observation = {stored_to->id(), offset, UnobservableStore observation = {stored_to->id(), offset};
is_initializing};
bool is_not_observable = uses.Contains(observation); bool is_not_observable = uses.Contains(observation);
if (is_not_observable) { if (is_not_observable) {
TRACE(" #%d is %sStoreField[+%d,%s](#%d), unobservable", node->id(), TRACE(" #%d is StoreField[+%d,%s](#%d), unobservable", node->id(),
(is_initializing ? "initializing " : ""), offset, offset, MachineReprToString(access.machine_type.representation()),
MachineReprToString(access.machine_type.representation()),
stored_to->id()); stored_to->id());
to_remove().insert(node); to_remove().insert(node);
return uses; return uses;
...@@ -326,16 +316,6 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet( ...@@ -326,16 +316,6 @@ UnobservablesSet RedundantStoreFinder::RecomputeSet(
return uses.RemoveSameOffset(offset, temp_zone()); 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 stores.
TRACE(
" #%d is Allocate or AllocateRaw, removing initalizing stores from "
"set",
node->id());
return uses.RemoveInitializingStores(temp_zone());
}
default: default:
if (CannotObserveStoreField(node)) { if (CannotObserveStoreField(node)) {
TRACE(" #%d:%s can observe nothing, set stays unchanged", node->id(), TRACE(" #%d:%s can observe nothing, set stays unchanged", node->id(),
...@@ -504,18 +484,6 @@ UnobservablesSet UnobservablesSet::RemoveSameOffset(StoreOffset offset, ...@@ -504,18 +484,6 @@ UnobservablesSet UnobservablesSet::RemoveSameOffset(StoreOffset offset,
return UnobservablesSet(new_set); return UnobservablesSet(new_set);
} }
UnobservablesSet UnobservablesSet::RemoveInitializingStores(Zone* zone) const {
UnobservablesSet::SetT* new_set = NewSet(zone);
*new_set = *set();
// Remove elements that are initializing stores.
for (auto entry : *new_set) {
const UnobservableStore& obs = entry.first;
if (obs.is_initializing_) SetErase(new_set, obs);
}
return UnobservablesSet(new_set);
}
} // namespace } // namespace
// static // static
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment