Revert "[VM/Compiler] Revoke definition status of certain instructions."
This reverts commit a8777149a78f16e60b63b917ba11c634c54e8fdc.
Reason for revert: benchmarking infrastructure reports broken benchmarks on ARM.
Original change's description:
> [VM/Compiler] Revoke definition status of certain instructions.
>
> Change StoreIndexed, StoreIndexedUnsafe, and StoreInstanceField
> to be non-definition instructions. Fix up the flow graph builders
> so they no longer treat StoreIndexed and StoreIndexedUnsafe as
> pushing values on the stack that need to be dropped.
>
> We recently changed FlowGraphInliner::TryInlineRecognizedMethod
> to create a distinction between the last instruction to be inlined
> and the definition that should be used as the new result. Perform
> the same separation of replacement code and result in the call
> specializers.
>
> Change-Id: I66c7ab60a662dcca05092e47156d1d4ba809d51f
> Reviewed-on: https://dart-review.googlesource.com/c/88326
> Commit-Queue: Stevie Strickland <sstrickl@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
TBR=kustermann@google.com,sstrickl@google.com
Change-Id: I9c666d80a69ba6a5c37b421aae1ed83502694f19
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/89366
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
diff --git a/runtime/vm/compiler/backend/constant_propagator.cc b/runtime/vm/compiler/backend/constant_propagator.cc
index cacf488..47e4be6 100644
--- a/runtime/vm/compiler/backend/constant_propagator.cc
+++ b/runtime/vm/compiler/backend/constant_propagator.cc
@@ -272,14 +272,6 @@
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.
}
@@ -692,6 +684,20 @@
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 16406e7..65d80af 100644
--- a/runtime/vm/compiler/backend/il.cc
+++ b/runtime/vm/compiler/backend/il.cc
@@ -1316,44 +1316,38 @@
return (kInstructionAttrs[tag()] & InstrAttrs::kNoGC) == 0;
}
-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 replacement's environment from this definition.
- ASSERT(replacement->env() == NULL);
- replacement->SetEnvironment(env());
- ClearEnv();
- // Replace all uses of this definition with replacement_for_uses.
- ReplaceUsesWith(replacement_for_uses);
-
- // Finally replace this one with the replacement instruction in the graph.
- previous()->LinkTo(replacement);
- if ((iterator != NULL) && (this == iterator->Current())) {
- // Remove through the iterator.
- replacement->LinkTo(this);
- iterator->RemoveCurrentFromGraph();
- } else {
- replacement->LinkTo(next());
- // Remove this definition's input uses.
- UnuseAllInputs();
- }
- set_previous(NULL);
- set_next(NULL);
-}
-
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);
+ input->definition()->AddInputUse(input);
+ }
+ // Take other's environment from this definition.
+ ASSERT(other->env() == NULL);
+ other->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());
}
- ReplaceWithResult(other, other, iterator);
+
+ // Finally insert the other definition in place of this one in the graph.
+ previous()->LinkTo(other);
+ if ((iterator != NULL) && (this == iterator->Current())) {
+ // Remove through the iterator.
+ other->LinkTo(this);
+ iterator->RemoveCurrentFromGraph();
+ } else {
+ other->LinkTo(next());
+ // Remove this definition's input uses.
+ UnuseAllInputs();
+ }
+ set_previous(NULL);
+ set_next(NULL);
}
void BranchInstr::SetComparison(ComparisonInstr* new_comparison) {
@@ -4998,7 +4992,7 @@
AlignmentType alignment,
intptr_t deopt_id,
TokenPosition token_pos)
- : TemplateInstruction(deopt_id),
+ : TemplateDefinition(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 a1550b2..7020590 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -1869,14 +1869,6 @@
// 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
@@ -2133,7 +2125,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 TemplateInstruction<2, NoThrow> {
+class StoreIndexedUnsafeInstr : public TemplateDefinition<2, NoThrow> {
public:
StoreIndexedUnsafeInstr(Value* index, Value* value, intptr_t offset)
: offset_(offset) {
@@ -4117,7 +4109,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 TemplateInstruction<2, NoThrow> {
+class StoreInstanceFieldInstr : public TemplateDefinition<2, NoThrow> {
public:
enum class Kind {
// Store is known to be the first store into a slot of an object after
@@ -4589,7 +4581,7 @@
DISALLOW_COPY_AND_ASSIGN(StringInterpolateInstr);
};
-class StoreIndexedInstr : public TemplateInstruction<3, NoThrow> {
+class StoreIndexedInstr : public TemplateDefinition<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 c44cd43..42beb69 100644
--- a/runtime/vm/compiler/backend/il_dbc.cc
+++ b/runtime/vm/compiler/backend/il_dbc.cc
@@ -1085,6 +1085,7 @@
}
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 42e2f9b..3d3404e 100644
--- a/runtime/vm/compiler/call_specializer.cc
+++ b/runtime/vm/compiler/call_specializer.cc
@@ -311,25 +311,14 @@
call->ReplaceWith(specialized, current_iterator());
}
-void CallSpecializer::ReplaceCallWithResult(Definition* call,
- Instruction* replacement,
- Definition* result) {
+void CallSpecializer::ReplaceCall(Definition* call, Definition* replacement) {
// 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();
}
- 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);
+ call->ReplaceWith(replacement, current_iterator());
}
void CallSpecializer::AddCheckSmi(Definition* to_check,
@@ -1076,7 +1065,7 @@
// Discard the environment from the original instruction because the store
// can't deoptimize.
instr->RemoveEnvironment();
- ReplaceCallWithResult(instr, store, flow_graph()->constant_null());
+ ReplaceCall(instr, store);
return true;
}
diff --git a/runtime/vm/compiler/call_specializer.h b/runtime/vm/compiler/call_specializer.h
index cb9ca8c..c830e02 100644
--- a/runtime/vm/compiler/call_specializer.h
+++ b/runtime/vm/compiler/call_specializer.h
@@ -83,12 +83,6 @@
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 eebb0b8..eadab31 100644
--- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc
@@ -451,6 +451,7 @@
Pop(), // Array.
index, value, emit_store_barrier, Instance::ElementSizeFor(class_id),
class_id, kAlignedAccess, DeoptId::kNone, TokenPosition::kNoSource);
+ Push(store);
return Fragment(store);
}
@@ -630,6 +631,7 @@
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 3562378..361da68 100644
--- a/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
+++ b/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
@@ -994,6 +994,7 @@
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 280ee7c..8bcbb49 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -694,6 +694,7 @@
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(
@@ -701,6 +702,7 @@
body += B->StoreFpRelativeSlot(
kWordSize *
(compiler_frame_layout.param_end_from_fp + function.NumParameters()));
+ body += Drop();
}
}
@@ -779,6 +781,7 @@
store += IntConstant(0);
store += LoadFunctionTypeArguments();
store += StoreIndexed(kArrayCid);
+ store += Drop();
store += IntConstant(1);
store += StoreLocal(TokenPosition::kNoSource, index);
store += Drop();
@@ -807,6 +810,7 @@
loop_body += B->LoadFpRelativeSlot(kWordSize *
compiler_frame_layout.param_end_from_fp);
loop_body += StoreIndexed(kArrayCid);
+ loop_body += Drop();
// ++i
loop_body += LoadLocal(index);
@@ -3155,6 +3159,7 @@
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.
@@ -3297,6 +3302,7 @@
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,
@@ -3854,6 +3860,7 @@
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;
}
@@ -3866,6 +3873,7 @@
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
@@ -3878,6 +3886,7 @@
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;
}
}
@@ -4289,6 +4298,7 @@
instructions += IntConstant(i);
instructions += BuildExpression(); // read ith expression.
instructions += StoreIndexed(kArrayCid);
+ instructions += Drop();
}
instructions += StringInterpolate(position);
@@ -4489,6 +4499,7 @@
instructions += IntConstant(i);
instructions += BuildExpression(); // read ith expression.
instructions += StoreIndexed(kArrayCid);
+ instructions += Drop();
}
}
instructions += PushArgument(); // The array.
@@ -4544,11 +4555,13 @@
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 dcc617c..cd0f7f7 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -1214,12 +1214,14 @@
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) {
@@ -1230,6 +1232,7 @@
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 0be271c..3b97b71 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());
- Instruction* cursor = replacement;
+ Definition* cursor = replacement;
Value* initial_value;
if (context_value != NULL) {
diff --git a/runtime/vm/simulator_dbc.cc b/runtime/vm/simulator_dbc.cc
index 230a519..2e97a9a 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 -= 2;
+ SP--;
DISPATCH();
}
