[dart/vm] Fix inconsistencies in intermediate representation
Rationale:
Stricter graph checking found some dangling pointers
and missing updates. This CL fixes the omissions
and removes some bail-outs in the graph checker itself,
making the checker stricter and more consistent. Also
adds flag to control verification (avoid excessive runtimes
for large programs in general, but allows testing even
these through command line option).
https://github.com/dart-lang/sdk/issues/36893
https://github.com/dart-lang/sdk/issues/36894
https://github.com/dart-lang/sdk/issues/36895
https://github.com/dart-lang/sdk/issues/36899
Change-Id: If4357cb897484ddfdb60722525092198771ec90a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/102420
Reviewed-by: Aart Bik <ajcbik@google.com>
Commit-Queue: Aart Bik <ajcbik@google.com>
diff --git a/runtime/vm/compiler/aot/aot_call_specializer.cc b/runtime/vm/compiler/aot/aot_call_specializer.cc
index 07d879f..895d8a7 100644
--- a/runtime/vm/compiler/aot/aot_call_specializer.cc
+++ b/runtime/vm/compiler/aot/aot_call_specializer.cc
@@ -1145,8 +1145,10 @@
InsertBefore(call, invoke_call->PushArgumentAt(i), NULL,
FlowGraph::kEffect);
}
- // Remove original PushArguments from the graph.
+ // Replace original PushArguments in the graph (mainly env uses).
+ ASSERT(call->ArgumentCount() == invoke_call->ArgumentCount());
for (intptr_t i = 0; i < call->ArgumentCount(); i++) {
+ call->PushArgumentAt(i)->ReplaceUsesWith(invoke_call->PushArgumentAt(i));
call->PushArgumentAt(i)->RemoveFromGraph();
}
diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc
index 406c5dd..3774ed8 100644
--- a/runtime/vm/compiler/backend/flow_graph.cc
+++ b/runtime/vm/compiler/backend/flow_graph.cc
@@ -153,6 +153,9 @@
void FlowGraph::AddToInitialDefinitions(BlockEntryWithInitialDefs* entry,
Definition* defn) {
defn->set_previous(entry);
+ if (auto par = defn->AsParameter()) {
+ par->set_block(entry); // set cached block
+ }
entry->initial_definitions()->Add(defn);
}
diff --git a/runtime/vm/compiler/backend/flow_graph_checker.cc b/runtime/vm/compiler/backend/flow_graph_checker.cc
index 242e3e2..8502f63 100644
--- a/runtime/vm/compiler/backend/flow_graph_checker.cc
+++ b/runtime/vm/compiler/backend/flow_graph_checker.cc
@@ -15,6 +15,20 @@
DECLARE_FLAG(bool, trace_compiler);
+DEFINE_FLAG(int,
+ verify_definitions_threshold,
+ 250,
+ "Definition count threshold for extensive instruction checks");
+
+// Returns true for the "optimized out" and "null" constant.
+static bool IsSpecialConstant(Definition* def) {
+ if (auto c = def->AsConstant()) {
+ return c->value().raw() == Symbols::OptimizedOut().raw() ||
+ c->value().raw() == Object::ZoneHandle().raw();
+ }
+ return false;
+}
+
// Returns true if block is a predecessor of succ.
static bool IsPred(BlockEntryInstr* block, BlockEntryInstr* succ) {
for (intptr_t i = 0, n = succ->PredecessorCount(); i < n; ++i) {
@@ -66,7 +80,7 @@
// checked by the Phi visitor below.
return true;
} else if (def->IsMaterializeObject() || instruction->IsMaterializeObject()) {
- // These instructions resides outside the IR.
+ // These instructions reside outside the IR.
return true;
} else if (auto entry =
instruction->GetBlock()->AsBlockEntryWithInitialDefs()) {
@@ -140,25 +154,20 @@
void FlowGraphChecker::VisitInstructions(BlockEntryInstr* block) {
// To avoid excessive runtimes, skip the instructions check if there
// are many definitions (as happens in e.g. an initialization block).
- if (flow_graph_->current_ssa_temp_index() > 10000) {
+ if (flow_graph_->current_ssa_temp_index() >
+ FLAG_verify_definitions_threshold) {
return;
}
// Give all visitors quick access.
current_block_ = block;
// Visit initial definitions.
if (auto entry = block->AsBlockEntryWithInitialDefs()) {
- GrowableArray<Definition*>* initial_defs = entry->initial_definitions();
- for (intptr_t i = 0, n = initial_defs->length(); i < n; i++) {
- Definition* def = (*initial_defs)[i];
+ for (auto def : *entry->initial_definitions()) {
ASSERT(def != nullptr);
ASSERT(def->IsConstant() || def->IsParameter() ||
def->IsSpecialParameter());
- // TODO(dartbug.com/36895) fix this bail-out
- // All null/optimized constants misbehave, parameters are moved
- // around without updating block_field.
- if (def->IsConstant() || def->GetBlock() != entry) {
- continue;
- }
+ // Special constants reside outside the IR.
+ if (IsSpecialConstant(def)) continue;
// Make sure block lookup agrees.
ASSERT(def->GetBlock() == entry);
// Initial definitions are partially linked into graph.
@@ -218,12 +227,10 @@
VisitUseDef(instruction, instruction->InputAt(i), i, /*is_env*/ false);
}
// Check all environment inputs.
- if (instruction->env() != nullptr) {
- intptr_t i = 0;
- for (Environment::DeepIterator it(instruction->env()); !it.Done();
- it.Advance()) {
- VisitUseDef(instruction, it.CurrentValue(), i++, /*is_env*/ true);
- }
+ intptr_t i = 0;
+ for (Environment::DeepIterator it(instruction->env()); !it.Done();
+ it.Advance()) {
+ VisitUseDef(instruction, it.CurrentValue(), i++, /*is_env*/ true);
}
// Visit specific instructions (definitions and anything with Visit()).
if (auto def = instruction->AsDefinition()) {
@@ -272,15 +279,17 @@
ASSERT(def->previous() == nullptr);
} else if (def->IsConstant() || def->IsParameter() ||
def->IsSpecialParameter()) {
- test_def = false; // TODO(dartbug.com/36895) fix this bail-out
+ // Initial definitions are partially linked into graph, but some
+ // constants are fully linked into graph (so no next() assert).
+ ASSERT(def->previous() != nullptr);
} else {
- if (is_env) return; // TODO(dartbug.com/36893)
// Others are fully linked into graph.
ASSERT(def->next() != nullptr);
ASSERT(def->previous() != nullptr);
}
if (test_def) {
- ASSERT(DefDominatesUse(def, instruction));
+ ASSERT(is_env || // TODO(dartbug.com/36899)
+ DefDominatesUse(def, instruction));
if (is_env) {
ASSERT(IsInUseList(def->env_use_list(), instruction));
} else {
@@ -343,14 +352,13 @@
}
void FlowGraphChecker::VisitPhi(PhiInstr* phi) {
- // Make sure each incoming input value of a Phi is dominated
- // on the corresponding incoming edge, as defined by order.
+ // Make sure the definition of each input value of a Phi dominates
+ // the corresponding incoming edge, as defined by order.
ASSERT(phi->InputCount() == current_block_->PredecessorCount());
for (intptr_t i = 0, n = phi->InputCount(); i < n; ++i) {
- Definition* input_def = phi->InputAt(i)->definition();
+ Definition* def = phi->InputAt(i)->definition();
BlockEntryInstr* edge = current_block_->PredecessorAt(i);
- ASSERT(input_def->IsConstant() || // TODO(dartbug.com/36894 and 36895)
- edge->last_instruction()->IsDominatedBy(input_def));
+ ASSERT(DefDominatesUse(def, edge->last_instruction()));
}
}
diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index cf93aaa..3e91b50 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -2160,6 +2160,7 @@
// Get the block entry for that instruction.
virtual BlockEntryInstr* GetBlock() { return block_; }
+ void set_block(BlockEntryInstr* block) { block_ = block; }
intptr_t InputCount() const { return 0; }
Value* InputAt(intptr_t i) const {
diff --git a/runtime/vm/compiler/backend/redundancy_elimination.cc b/runtime/vm/compiler/backend/redundancy_elimination.cc
index f0fa894..27245b2 100644
--- a/runtime/vm/compiler/backend/redundancy_elimination.cc
+++ b/runtime/vm/compiler/backend/redundancy_elimination.cc
@@ -3549,6 +3549,7 @@
new (flow_graph_->zone()) ConstantInstr(orig->value());
copy->set_ssa_temp_index(flow_graph_->alloc_ssa_temp_index());
old->ReplaceUsesWith(copy);
+ copy->set_previous(old->previous()); // partial link
(*idefs)[j] = copy;
}
}
