languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-10-14 12:30:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

[vm/compiler] Do not use Constant(#null) to reserve space on the expression stack.

Browse source 
Given IL like this:

  t3 <- Constant(#null)   <--\
  // ... do something ...    |
  StoreLocal(:temp, t4)   ---/
  // Do something else
  StoreIndexed(t1, t2, t3)

Graph builder would use Constant(#null) as an input definition for
StoreIndexed - which leads to wrong generated code in unoptimized mode
on X64 where StoreIndexed has specialization for constant values. In
optimized code SSA renaming would actually replace t3 with a correct
reaching definition.

This CL introduces a new IL instruction MakeTemp(...) which has exactly
the same effect as Constant(#null) on the expression stack, but is
not treated as a constant value by the backend.

The bug was unnoticed because of the loose assertions in the SSA
renaming which permitted replacing Constant-s with non-Constant
reaching definitions. This CL also tightens those assertions to
catch this issue.

Also cleanup the code in SSA renaming a bit to make it a bit easier
to understand.

Fixes https://github.com/dart-lang/sdk/issues/33195

Bug: 33195
Change-Id: I7c914c836d4af7a50e8a8e1a02d03e9413f87779
Reviewed-on: https://dart-review.googlesource.com/56112
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
This commit is contained in:
Vyacheslav Egorov 2018-05-22 18:05:14 +00:00 committed by commit-bot@chromium.org
parent a756248065
commit 3f6c87d5ac
9 changed files with 196 additions and 94 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
runtime/vm/compiler/backend/constant_propagator.cc
View file

@ -386,6 +386,11 @@ void ConstantPropagator::VisitDropTemps(DropTempsInstr* instr) {
UNREACHABLE();
}
void ConstantPropagator::VisitMakeTemp(MakeTempInstr* instr) {
// Instruction is eliminated when translating to SSA.
UNREACHABLE();
}
void ConstantPropagator::VisitStoreLocal(StoreLocalInstr* instr) {
// Instruction is eliminated when translating to SSA.
UNREACHABLE();

199
runtime/vm/compiler/backend/flow_graph.cc
View file

@ -1167,8 +1167,8 @@ void FlowGraph::RenameRecursive(BlockEntryInstr* block_entry,
// 2a. Handle uses:
// Update the expression stack renaming environment for each use by
// removing the renamed value.
// For each use of a LoadLocal, StoreLocal, DropTemps or Constant: Replace
// it with the renamed value.
// For each use of a LoadLocal, StoreLocal, MakeTemp, DropTemps or Constant:
// replace it with the renamed value.
for (intptr_t i = current->InputCount() - 1; i >= 0; --i) {
Value* v = current->InputAt(i);
// Update expression stack.
@ -1177,8 +1177,10 @@ void FlowGraph::RenameRecursive(BlockEntryInstr* block_entry,
Definition* reaching_defn = env->RemoveLast();
Definition* input_defn = v->definition();
if (input_defn != reaching_defn) {
// Note: constants can only be replaced with other constants.
ASSERT(input_defn->IsLoadLocal() || input_defn->IsStoreLocal() ||
input_defn->IsDropTemps() || input_defn->IsConstant());
input_defn->IsDropTemps() || input_defn->IsMakeTemp() ||
(input_defn->IsConstant() && reaching_defn->IsConstant()));
// Assert we are not referencing nulls in the initial environment.
ASSERT(reaching_defn->ssa_temp_index() != -1);
v->set_definition(reaching_defn);
@ -1192,103 +1194,120 @@ void FlowGraph::RenameRecursive(BlockEntryInstr* block_entry,
env->RemoveLast();
}
// 2b. Handle LoadLocal, StoreLocal, DropTemps and Constant.
Definition* definition = current->AsDefinition();
if (definition != NULL) {
LoadLocalInstr* load = definition->AsLoadLocal();
StoreLocalInstr* store = definition->AsStoreLocal();
DropTempsInstr* drop = definition->AsDropTemps();
ConstantInstr* constant = definition->AsConstant();
if ((load != NULL) || (store != NULL) || (drop != NULL) ||
(constant != NULL)) {
Definition* result = NULL;
if (store != NULL) {
// Update renaming environment.
intptr_t index = store->local().BitIndexIn(num_direct_parameters_);
result = store->value()->definition();
// 2b. Handle LoadLocal/StoreLocal/MakeTemp/DropTemps/Constant and
// PushArgument specially. Other definitions are just pushed
// to the environment directly.
Definition* result = NULL;
switch (current->tag()) {
case Instruction::kLoadLocal: {
LoadLocalInstr* load = current->Cast<LoadLocalInstr>();
// The graph construction ensures we do not have an unused LoadLocal
// computation.
ASSERT(load->HasTemp());
const intptr_t index = load->local().BitIndexIn(num_direct_parameters_);
result = (*env)[index];
if (!FLAG_prune_dead_locals ||
variable_liveness->IsStoreAlive(block_entry, store)) {
(*env)[index] = result;
} else {
(*env)[index] = constant_dead();
}
} else if (load != NULL) {
// The graph construction ensures we do not have an unused LoadLocal
// computation.
ASSERT(definition->HasTemp());
intptr_t index = load->local().BitIndexIn(num_direct_parameters_);
result = (*env)[index];
PhiInstr* phi = result->AsPhi();
if ((phi != NULL) && !phi->is_alive()) {
phi->mark_alive();
live_phis->Add(phi);
}
PhiInstr* phi = result->AsPhi();
if ((phi != NULL) && !phi->is_alive()) {
phi->mark_alive();
live_phis->Add(phi);
}
if (FLAG_prune_dead_locals &&
variable_liveness->IsLastLoad(block_entry, load)) {
(*env)[index] = constant_dead();
}
if (FLAG_prune_dead_locals &&
variable_liveness->IsLastLoad(block_entry, load)) {
(*env)[index] = constant_dead();
}
// Record captured parameters so that they can be skipped when
// emitting sync code inside optimized try-blocks.
if (load->local().is_captured_parameter()) {
captured_parameters_->Add(index);
}
// Record captured parameters so that they can be skipped when
// emitting sync code inside optimized try-blocks.
if (load->local().is_captured_parameter()) {
intptr_t index = load->local().BitIndexIn(num_direct_parameters_);
captured_parameters_->Add(index);
}
if ((phi != NULL) && isolate()->strong() &&
FLAG_use_strong_mode_types) {
// Assign type to Phi if it doesn't have a type yet.
// For a Phi to appear in the local variable it either was placed
// there as incoming value by renaming or it was stored there by
// StoreLocal which took this Phi from another local via LoadLocal,
// to which this reasoning applies recursively.
// This means that we are guaranteed to process LoadLocal for a
// matching variable first.
if (!phi->HasType()) {
ASSERT((index < phi->block()->phis()->length()) &&
((*phi->block()->phis())[index] == phi));
phi->UpdateType(
CompileType::FromAbstractType(load->local().type()));
}
}
} else if (drop != NULL) {
// Drop temps from the environment.
for (intptr_t j = 0; j < drop->num_temps(); j++) {
env->RemoveLast();
}
if (drop->value() != NULL) {
result = drop->value()->definition();
}
ASSERT((drop->value() != NULL) || !drop->HasTemp());
} else {
if (definition->HasTemp()) {
result = GetConstant(constant->value());
if ((phi != NULL) && isolate()->strong() &&
FLAG_use_strong_mode_types) {
// Assign type to Phi if it doesn't have a type yet.
// For a Phi to appear in the local variable it either was placed
// there as incoming value by renaming or it was stored there by
// StoreLocal which took this Phi from another local via LoadLocal,
// to which this reasoning applies recursively.
// This means that we are guaranteed to process LoadLocal for a
// matching variable first.
if (!phi->HasType()) {
ASSERT((index < phi->block()->phis()->length()) &&
((*phi->block()->phis())[index] == phi));
phi->UpdateType(
CompileType::FromAbstractType(load->local().type()));
}
}
// Update expression stack or remove from graph.
if (definition->HasTemp()) {
ASSERT(result != NULL);
env->Add(result);
}
it.RemoveCurrentFromGraph();
} else {
// Not a load, store, drop or constant.
if (definition->HasTemp()) {
// Assign fresh SSA temporary and update expression stack.
AllocateSSAIndexes(definition);
env->Add(definition);
}
break;
}
case Instruction::kStoreLocal: {
StoreLocalInstr* store = current->Cast<StoreLocalInstr>();
const intptr_t index =
store->local().BitIndexIn(num_direct_parameters_);
result = store->value()->definition();
if (!FLAG_prune_dead_locals ||
variable_liveness->IsStoreAlive(block_entry, store)) {
(*env)[index] = result;
} else {
(*env)[index] = constant_dead();
}
break;
}
case Instruction::kDropTemps: {
// Drop temps from the environment.
DropTempsInstr* drop = current->Cast<DropTempsInstr>();
for (intptr_t j = 0; j < drop->num_temps(); j++) {
env->RemoveLast();
}
if (drop->value() != NULL) {
result = drop->value()->definition();
}
ASSERT((drop->value() != NULL) || !drop->HasTemp());
break;
}
case Instruction::kConstant: {
ConstantInstr* constant = current->Cast<ConstantInstr>();
if (constant->HasTemp()) {
result = GetConstant(constant->value());
}
break;
}
case Instruction::kMakeTemp: {
// Simply push a #null value to the expression stack.
result = constant_null_;
break;
}
case Instruction::kPushArgument:
env->Add(current->Cast<PushArgumentInstr>());
continue;
default:
// Other definitions directly go into the environment.
if (Definition* definition = current->AsDefinition()) {
if (definition->HasTemp()) {
// Assign fresh SSA temporary and update expression stack.
AllocateSSAIndexes(definition);
env->Add(definition);
}
}
continue;
}
// 2c. Handle pushed argument.
PushArgumentInstr* push = current->AsPushArgument();
if (push != NULL) {
env->Add(push);
// Update expression stack and remove current instruction from the graph.
Definition* definition = current->Cast<Definition>();
if (definition->HasTemp()) {
ASSERT(result != NULL);
env->Add(result);
}
it.RemoveCurrentFromGraph();
}
// 3. Process dominated blocks.

13
runtime/vm/compiler/backend/il.cc
View file

@ -3377,8 +3377,21 @@ void SpecialParameterInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
UNREACHABLE();
}
LocationSummary* MakeTempInstr::MakeLocationSummary(Zone* zone,
bool optimizing) const {
ASSERT(!optimizing);
null_->InitializeLocationSummary(zone, optimizing);
return null_->locs();
}
void MakeTempInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
ASSERT(!compiler->is_optimizing());
null_->EmitNativeCode(compiler);
}
LocationSummary* DropTempsInstr::MakeLocationSummary(Zone* zone,
bool optimizing) const {
ASSERT(!optimizing);
return (InputCount() == 1)
? LocationSummary::Make(zone, 1, Location::SameAsFirstInput(),
LocationSummary::kNoCall)

51
runtime/vm/compiler/backend/il.h
View file

@ -488,6 +488,7 @@ class EmbeddedArray<T, 0> {
M(StaticCall) \
M(LoadLocal) \
M(DropTemps) \
M(MakeTemp) \
M(StoreLocal) \
M(StrictCompare) \
M(EqualityCompare) \
@ -873,6 +874,11 @@ class Instruction : public ZoneAllocated {
#undef INSTRUCTION_TYPE_CHECK
#undef DECLARE_INSTRUCTION_TYPE_CHECK
template <typename T>
T* Cast() {
return static_cast<T*>(this);
}
// Returns structure describing location constraints required
// to emit native code for this instruction.
LocationSummary* locs() {
@ -3898,6 +3904,51 @@ class DropTempsInstr : public Definition {
DISALLOW_COPY_AND_ASSIGN(DropTempsInstr);
};
// This instruction is used to reserve a space on the expression stack
// that later would be filled with StoreLocal. Reserved space would be
// filled with a null value initially.
//
// Note: One must not use Constant(#null) to reserve expression stack space
// because it would lead to an incorrectly compiled unoptimized code. Graph
// builder would set Constant(#null) as an input definition to the instruction
// that consumes this value from the expression stack - not knowing that
// this value represents a placeholder - which might lead issues if instruction
// has specialization for constant inputs (see https://dartbug.com/33195).
class MakeTempInstr : public TemplateDefinition<0, NoThrow, Pure> {
public:
explicit MakeTempInstr(Zone* zone)
: null_(new (zone) ConstantInstr(Object::ZoneHandle())) {
// Note: We put ConstantInstr inside MakeTemp to simplify code generation:
// having ConstantInstr allows us to use Location::Contant(null_) as an
// output location for this instruction.
}
DECLARE_INSTRUCTION(MakeTemp)
virtual CompileType ComputeType() const { return CompileType::Dynamic(); }
virtual bool ComputeCanDeoptimize() const { return false; }
virtual bool HasUnknownSideEffects() const {
UNREACHABLE(); // Eliminated by SSA construction.
return false;
}
virtual bool MayThrow() const {
UNREACHABLE();
return false;
}
virtual TokenPosition token_pos() const { return TokenPosition::kTempMove; }
PRINT_OPERANDS_TO_SUPPORT
private:
ConstantInstr* null_;
DISALLOW_COPY_AND_ASSIGN(MakeTempInstr);
};
class StoreLocalInstr : public TemplateDefinition<1, NoThrow> {
public:
StoreLocalInstr(const LocalVariable& local,

2
runtime/vm/compiler/backend/il_printer.cc
View file

@ -447,6 +447,8 @@ const char* RangeBoundary::ToCString() const {
return Thread::Current()->zone()->MakeCopyOfString(buffer);
}
void MakeTempInstr::PrintOperandsTo(BufferFormatter* f) const {}
void DropTempsInstr::PrintOperandsTo(BufferFormatter* f) const {
f->Print("%" Pd "", num_temps());
if (value() != NULL) {

12
runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
View file

@ -5669,7 +5669,7 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder(
function_node_helper.ReadUntilExcluding(
FunctionNodeHelper::kPositionalParameters);
body += NullConstant();
body += MakeTemp();
LocalVariable* result = MakeTemporary();
// Do "++argument_count" if any type arguments were passed.
@ -7796,6 +7796,10 @@ Fragment StreamingFlowGraphBuilder::DropTempsPreserveTop(
return flow_graph_builder_->DropTempsPreserveTop(num_temps_to_drop);
}
Fragment StreamingFlowGraphBuilder::MakeTemp() {
return flow_graph_builder_->MakeTemp();
}
Fragment StreamingFlowGraphBuilder::NullConstant() {
return flow_graph_builder_->NullConstant();
}
@ -8067,7 +8071,7 @@ Fragment StreamingFlowGraphBuilder::BuildPropertySet(TokenPosition* p) {
const DirectCallMetadata direct_call =
direct_call_metadata_helper_.GetDirectTargetForPropertySet(offset);
Fragment instructions(NullConstant());
Fragment instructions(MakeTemp());
LocalVariable* variable = MakeTemporary();
const TokenPosition position = ReadPosition(); // read position.
@ -8285,7 +8289,7 @@ Fragment StreamingFlowGraphBuilder::BuildSuperPropertySet(TokenPosition* p) {
Function& function = FindMatchingFunctionAnyArgs(klass, setter_name);
Fragment instructions(NullConstant());
Fragment instructions(MakeTemp());
LocalVariable* value = MakeTemporary(); // this holds RHS value
if (function.IsNull()) {
@ -8395,7 +8399,7 @@ Fragment StreamingFlowGraphBuilder::BuildDirectPropertySet(TokenPosition* p) {
const TokenPosition position = ReadPosition(); // read position.
if (p != NULL) *p = position;
Fragment instructions(NullConstant());
Fragment instructions(MakeTemp());
LocalVariable* value = MakeTemporary();
instructions += BuildExpression(); // read receiver.

1
runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
View file

@ -1455,6 +1455,7 @@ class StreamingFlowGraphBuilder : public KernelReaderHelper {
// Drop given number of temps from the stack but preserve top of the stack.
Fragment DropTempsPreserveTop(intptr_t num_temps_to_drop);
Fragment MakeTemp();
Fragment NullConstant();
JoinEntryInstr* BuildJoinEntry();
JoinEntryInstr* BuildJoinEntry(intptr_t try_index);

6
runtime/vm/compiler/frontend/kernel_to_il.cc
View file

@ -2040,6 +2040,12 @@ Fragment BaseFlowGraphBuilder::DropTempsPreserveTop(
return Fragment(drop_temps);
}
Fragment BaseFlowGraphBuilder::MakeTemp() {
MakeTempInstr* make_temp = new (Z) MakeTempInstr(Z);
Push(make_temp);
return Fragment(make_temp);
}
void FlowGraphBuilder::InlineBailout(const char* reason) {
bool is_inlining = exit_collector_ != NULL;
if (is_inlining) {

1
runtime/vm/compiler/frontend/kernel_to_il.h
View file

@ -580,6 +580,7 @@ class BaseFlowGraphBuilder {
Fragment Drop();
// Drop given number of temps from the stack but preserve top of the stack.
Fragment DropTempsPreserveTop(intptr_t num_temps_to_drop);
Fragment MakeTemp();
// Create a pseudo-local variable for a location on the expression stack.
// Note: SSA construction currently does not support inserting Phi functions