Reland [VM/Compiler] Removing definition status from Store{Indexed,InstanceField}.
Also do the same for the unsafe version of StoreIndexed.
Similar to the inliner, we may replace calls in the specializers with
these non-definition instructions, so we make variants that allow an
unrelated value to be used for the old users of the call site value. (In
all cases, this ends up being null, since the original calls wouldn't
have returned a useable value anyway.)
Change-Id: I0befd54920c4c0e63e086b804860ee4c9a7d3767
Reviewed-on: https://dart-review.googlesource.com/c/89585
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Stevie Strickland <sstrickl@google.com>
diff --git a/runtime/vm/compiler/backend/constant_propagator.cc b/runtime/vm/compiler/backend/constant_propagator.cc
index 47e4be6..cacf488 100644
--- a/runtime/vm/compiler/backend/constant_propagator.cc
+++ b/runtime/vm/compiler/backend/constant_propagator.cc
@@ -272,6 +272,14 @@
void ConstantPropagator::VisitCheckEitherNonSmi(CheckEitherNonSmiInstr* instr) {
}
+void ConstantPropagator::VisitStoreIndexedUnsafe(
+ StoreIndexedUnsafeInstr* instr) {}
+
+void ConstantPropagator::VisitStoreIndexed(StoreIndexedInstr* instr) {}
+
+void ConstantPropagator::VisitStoreInstanceField(
+ StoreInstanceFieldInstr* instr) {}
+
void ConstantPropagator::VisitDeoptimize(DeoptimizeInstr* instr) {
// TODO(vegorov) remove all code after DeoptimizeInstr as dead.
}
@@ -684,20 +692,6 @@
SetValue(instr, non_constant_);
}
-void ConstantPropagator::VisitStoreIndexedUnsafe(
- StoreIndexedUnsafeInstr* instr) {
- SetValue(instr, non_constant_);
-}
-
-void ConstantPropagator::VisitStoreIndexed(StoreIndexedInstr* instr) {
- SetValue(instr, instr->value()->definition()->constant_value());
-}
-
-void ConstantPropagator::VisitStoreInstanceField(
- StoreInstanceFieldInstr* instr) {
- SetValue(instr, instr->value()->definition()->constant_value());
-}
-
void ConstantPropagator::VisitInitStaticField(InitStaticFieldInstr* instr) {
// Nothing to do.
}
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
index 65d80af..16406e7 100644
--- a/runtime/vm/compiler/backend/il.cc
+++ b/runtime/vm/compiler/backend/il.cc
@@ -1316,33 +1316,29 @@
return (kInstructionAttrs[tag()] & InstrAttrs::kNoGC) == 0;
}
-void Definition::ReplaceWith(Definition* other,
- ForwardInstructionIterator* iterator) {
- // Record other's input uses.
- for (intptr_t i = other->InputCount() - 1; i >= 0; --i) {
- Value* input = other->InputAt(i);
+void Definition::ReplaceWithResult(Instruction* replacement,
+ Definition* replacement_for_uses,
+ ForwardInstructionIterator* iterator) {
+ // Record replacement's input uses.
+ for (intptr_t i = replacement->InputCount() - 1; i >= 0; --i) {
+ Value* input = replacement->InputAt(i);
input->definition()->AddInputUse(input);
}
- // Take other's environment from this definition.
- ASSERT(other->env() == NULL);
- other->SetEnvironment(env());
+ // Take replacement's environment from this definition.
+ ASSERT(replacement->env() == NULL);
+ replacement->SetEnvironment(env());
ClearEnv();
- // Replace all uses of this definition with other.
- ReplaceUsesWith(other);
- // Reuse this instruction's SSA name for other.
- ASSERT(!other->HasSSATemp());
- if (HasSSATemp()) {
- other->set_ssa_temp_index(ssa_temp_index());
- }
+ // Replace all uses of this definition with replacement_for_uses.
+ ReplaceUsesWith(replacement_for_uses);
- // Finally insert the other definition in place of this one in the graph.
- previous()->LinkTo(other);
+ // Finally replace this one with the replacement instruction in the graph.
+ previous()->LinkTo(replacement);
if ((iterator != NULL) && (this == iterator->Current())) {
// Remove through the iterator.
- other->LinkTo(this);
+ replacement->LinkTo(this);
iterator->RemoveCurrentFromGraph();
} else {
- other->LinkTo(next());
+ replacement->LinkTo(next());
// Remove this definition's input uses.
UnuseAllInputs();
}
@@ -1350,6 +1346,16 @@
set_next(NULL);
}
+void Definition::ReplaceWith(Definition* other,
+ ForwardInstructionIterator* iterator) {
+ // Reuse this instruction's SSA name for other.
+ ASSERT(!other->HasSSATemp());
+ if (HasSSATemp()) {
+ other->set_ssa_temp_index(ssa_temp_index());
+ }
+ ReplaceWithResult(other, other, iterator);
+}
+
void BranchInstr::SetComparison(ComparisonInstr* new_comparison) {
for (intptr_t i = new_comparison->InputCount() - 1; i >= 0; --i) {
Value* input = new_comparison->InputAt(i);
@@ -4992,7 +4998,7 @@
AlignmentType alignment,
intptr_t deopt_id,
TokenPosition token_pos)
- : TemplateDefinition(deopt_id),
+ : TemplateInstruction(deopt_id),
emit_store_barrier_(emit_store_barrier),
index_scale_(index_scale),
class_id_(class_id),
diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index 7020590..a1550b2 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -1869,6 +1869,14 @@
// Postcondition: use lists and use values are still valid.
void ReplaceUsesWith(Definition* other);
+ // Replace this definition with another instruction. Use the provided result
+ // definition to replace uses of the original definition. If replacing during
+ // iteration, pass the iterator so that the instruction can be replaced
+ // without affecting iteration order, otherwise pass a NULL iterator.
+ void ReplaceWithResult(Instruction* replacement,
+ Definition* replacement_for_uses,
+ ForwardInstructionIterator* iterator);
+
// Replace this definition and all uses with another definition. If
// replacing during iteration, pass the iterator so that the instruction
// can be replaced without affecting iteration order, otherwise pass a
@@ -2125,7 +2133,7 @@
//
// This low-level instruction is non-inlinable since it makes assumptions about
// the frame. This is asserted via `inliner.cc::CalleeGraphValidator`.
-class StoreIndexedUnsafeInstr : public TemplateDefinition<2, NoThrow> {
+class StoreIndexedUnsafeInstr : public TemplateInstruction<2, NoThrow> {
public:
StoreIndexedUnsafeInstr(Value* index, Value* value, intptr_t offset)
: offset_(offset) {
@@ -4109,7 +4117,7 @@
// field initializers *must* be marked as initializing. Initializing stores
// into unboxed fields are responsible for allocating the mutable box which
// would be mutated by subsequent stores.
-class StoreInstanceFieldInstr : public TemplateDefinition<2, NoThrow> {
+class StoreInstanceFieldInstr : public TemplateInstruction<2, NoThrow> {
public:
enum class Kind {
// Store is known to be the first store into a slot of an object after
@@ -4581,7 +4589,7 @@
DISALLOW_COPY_AND_ASSIGN(StringInterpolateInstr);
};
-class StoreIndexedInstr : public TemplateDefinition<3, NoThrow> {
+class StoreIndexedInstr : public TemplateInstruction<3, NoThrow> {
public:
StoreIndexedInstr(Value* array,
Value* index,
diff --git a/runtime/vm/compiler/backend/il_dbc.cc b/runtime/vm/compiler/backend/il_dbc.cc
index 42beb69..c44cd43 100644
--- a/runtime/vm/compiler/backend/il_dbc.cc
+++ b/runtime/vm/compiler/backend/il_dbc.cc
@@ -1085,7 +1085,6 @@
}
EMIT_NATIVE_CODE(StoreInstanceField, 2) {
- ASSERT(!HasTemp());
ASSERT(OffsetInBytes() % kWordSize == 0);
if (compiler->is_optimizing()) {
const Register value = locs()->in(1).reg();
diff --git a/runtime/vm/compiler/call_specializer.cc b/runtime/vm/compiler/call_specializer.cc
index 3d3404e..42e2f9b 100644
--- a/runtime/vm/compiler/call_specializer.cc
+++ b/runtime/vm/compiler/call_specializer.cc
@@ -311,14 +311,25 @@
call->ReplaceWith(specialized, current_iterator());
}
-void CallSpecializer::ReplaceCall(Definition* call, Definition* replacement) {
+void CallSpecializer::ReplaceCallWithResult(Definition* call,
+ Instruction* replacement,
+ Definition* result) {
// Remove the original push arguments.
for (intptr_t i = 0; i < call->ArgumentCount(); ++i) {
PushArgumentInstr* push = call->PushArgumentAt(i);
push->ReplaceUsesWith(push->value()->definition());
push->RemoveFromGraph();
}
- call->ReplaceWith(replacement, current_iterator());
+ if (result == nullptr) {
+ ASSERT(replacement->IsDefinition());
+ call->ReplaceWith(replacement->AsDefinition(), current_iterator());
+ } else {
+ call->ReplaceWithResult(replacement, result, current_iterator());
+ }
+}
+
+void CallSpecializer::ReplaceCall(Definition* call, Definition* replacement) {
+ ReplaceCallWithResult(call, replacement, nullptr);
}
void CallSpecializer::AddCheckSmi(Definition* to_check,
@@ -1065,7 +1076,7 @@
// Discard the environment from the original instruction because the store
// can't deoptimize.
instr->RemoveEnvironment();
- ReplaceCall(instr, store);
+ ReplaceCallWithResult(instr, store, flow_graph()->constant_null());
return true;
}
diff --git a/runtime/vm/compiler/call_specializer.h b/runtime/vm/compiler/call_specializer.h
index c830e02..cb9ca8c 100644
--- a/runtime/vm/compiler/call_specializer.h
+++ b/runtime/vm/compiler/call_specializer.h
@@ -83,6 +83,12 @@
bool TryInlineInstanceMethod(InstanceCallInstr* call);
void ReplaceWithInstanceOf(InstanceCallInstr* instr);
+ // Replaces a call where the replacement code does not end in a
+ // value-returning instruction, so we must specify what definition should be
+ // used instead to replace uses of the call return value.
+ void ReplaceCallWithResult(Definition* call,
+ Instruction* replacement,
+ Definition* result);
void ReplaceCall(Definition* call, Definition* replacement);
// Add a class check for the call's first argument (receiver).
diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
index eadab31..eebb0b8 100644
--- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
@@ -451,7 +451,6 @@
Pop(), // Array.
index, value, emit_store_barrier, Instance::ElementSizeFor(class_id),
class_id, kAlignedAccess, DeoptId::kNone, TokenPosition::kNoSource);
- Push(store);
return Fragment(store);
}
@@ -631,7 +630,6 @@
Value* index = Pop();
StoreIndexedUnsafeInstr* instr =
new (Z) StoreIndexedUnsafeInstr(index, value, offset);
- Push(instr);
return Fragment(instr);
}
diff --git a/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc b/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
index 361da68..3562378 100644
--- a/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
@@ -994,7 +994,6 @@
void BytecodeFlowGraphBuilder::BuildStoreIndexedTOS() {
LoadStackSlots(3);
code_ += B->StoreIndexed(kArrayCid);
- code_ += B->Drop();
}
void BytecodeFlowGraphBuilder::BuildBooleanNegateTOS() {
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
index 8bcbb49..280ee7c 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -694,7 +694,6 @@
Slot::GetContextVariableSlotFor(thread(), *scopes()->this_variable));
body += B->StoreFpRelativeSlot(kWordSize *
compiler_frame_layout.param_end_from_fp);
- body += Drop();
} else {
body += LoadLocal(parsed_function()->current_context_var());
body += B->LoadNativeField(
@@ -702,7 +701,6 @@
body += B->StoreFpRelativeSlot(
kWordSize *
(compiler_frame_layout.param_end_from_fp + function.NumParameters()));
- body += Drop();
}
}
@@ -781,7 +779,6 @@
store += IntConstant(0);
store += LoadFunctionTypeArguments();
store += StoreIndexed(kArrayCid);
- store += Drop();
store += IntConstant(1);
store += StoreLocal(TokenPosition::kNoSource, index);
store += Drop();
@@ -810,7 +807,6 @@
loop_body += B->LoadFpRelativeSlot(kWordSize *
compiler_frame_layout.param_end_from_fp);
loop_body += StoreIndexed(kArrayCid);
- loop_body += Drop();
// ++i
loop_body += LoadLocal(index);
@@ -3159,7 +3155,6 @@
instructions += IntConstant(num_type_arguments == 0 ? 0 : 1); // index
instructions += LoadLocal(scopes()->this_variable); // receiver
instructions += StoreIndexed(kArrayCid);
- instructions += Drop(); // dispose of stored value
instructions += build_rest_of_actuals;
// First argument is receiver.
@@ -3302,7 +3297,6 @@
build_rest_of_actuals += BuildExpression(); // value.
build_rest_of_actuals += StoreLocal(position, value);
build_rest_of_actuals += StoreIndexed(kArrayCid);
- build_rest_of_actuals += Drop(); // dispose of stored value
instructions += BuildAllocateInvocationMirrorCall(
position, setter_name, /* num_type_arguments = */ 0,
@@ -3860,7 +3854,6 @@
build_rest_of_actuals +=
TranslateInstantiatedTypeArguments(type_arguments);
build_rest_of_actuals += StoreIndexed(kArrayCid);
- build_rest_of_actuals += Drop(); // dispose of stored value
++actuals_array_index;
}
@@ -3873,7 +3866,6 @@
build_rest_of_actuals += IntConstant(actuals_array_index + i); // index
build_rest_of_actuals += BuildExpression(); // value.
build_rest_of_actuals += StoreIndexed(kArrayCid);
- build_rest_of_actuals += Drop(); // dispose of stored value
++i;
}
// Read named arguments
@@ -3886,7 +3878,6 @@
build_rest_of_actuals += IntConstant(i + actuals_array_index); // index
build_rest_of_actuals += BuildExpression(); // value.
build_rest_of_actuals += StoreIndexed(kArrayCid);
- build_rest_of_actuals += Drop(); // dispose of stored value
++i;
}
}
@@ -4298,7 +4289,6 @@
instructions += IntConstant(i);
instructions += BuildExpression(); // read ith expression.
instructions += StoreIndexed(kArrayCid);
- instructions += Drop();
}
instructions += StringInterpolate(position);
@@ -4499,7 +4489,6 @@
instructions += IntConstant(i);
instructions += BuildExpression(); // read ith expression.
instructions += StoreIndexed(kArrayCid);
- instructions += Drop();
}
}
instructions += PushArgument(); // The array.
@@ -4555,13 +4544,11 @@
instructions += IntConstant(2 * i);
instructions += BuildExpression(); // read ith key.
instructions += StoreIndexed(kArrayCid);
- instructions += Drop();
instructions += LoadLocal(array);
instructions += IntConstant(2 * i + 1);
instructions += BuildExpression(); // read ith value.
instructions += StoreIndexed(kArrayCid);
- instructions += Drop();
}
}
instructions += PushArgument(); // The array.
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc
index cd0f7f7..dcc617c 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -1214,14 +1214,12 @@
body += IntConstant(0);
body += LoadLocal(type_args);
body += StoreIndexed(kArrayCid);
- body += Drop();
}
for (intptr_t i = 0; i < descriptor.PositionalCount(); ++i) {
body += LoadLocal(array);
body += IntConstant(receiver_index + i);
body += LoadLocal(scope->VariableAt(i));
body += StoreIndexed(kArrayCid);
- body += Drop();
}
String& name = String::Handle(Z);
for (intptr_t i = 0; i < descriptor.NamedCount(); ++i) {
@@ -1232,7 +1230,6 @@
body += IntConstant(receiver_index + descriptor.PositionAt(i));
body += LoadLocal(scope->VariableAt(parameter_index));
body += StoreIndexed(kArrayCid);
- body += Drop();
}
body += PushArgument();
diff --git a/runtime/vm/compiler/jit/jit_call_specializer.cc b/runtime/vm/compiler/jit/jit_call_specializer.cc
index 3b97b71..0be271c 100644
--- a/runtime/vm/compiler/jit/jit_call_specializer.cc
+++ b/runtime/vm/compiler/jit/jit_call_specializer.cc
@@ -244,7 +244,7 @@
context_variables.length());
alloc->ReplaceWith(replacement, current_iterator());
- Definition* cursor = replacement;
+ Instruction* cursor = replacement;
Value* initial_value;
if (context_value != NULL) {
diff --git a/runtime/vm/simulator_dbc.cc b/runtime/vm/simulator_dbc.cc
index 2e97a9a..230a519 100644
--- a/runtime/vm/simulator_dbc.cc
+++ b/runtime/vm/simulator_dbc.cc
@@ -3758,7 +3758,7 @@
RawSmi* index = RAW_CAST(Smi, SP[-1]);
const int16_t offset = rD;
FP[-(Smi::Value(index) + offset) - 0] = SP[-0];
- SP--;
+ SP -= 2;
DISPATCH();
}
