Cleanup handling of NullType in type propagation.

runtime/vm/flow_graph_optimizer.cc

Index: runtime/vm/flow_graph_optimizer.cc
===================================================================
--- runtime/vm/flow_graph_optimizer.cc	(revision 10752)
+++ runtime/vm/flow_graph_optimizer.cc	(working copy)
@@ -596,9 +596,26 @@
                                                     BindInstr* instr) {
   if (FLAG_eliminate_type_checks &&
       !comp->is_eliminated() &&
-      !comp->dst_type().IsMalformed() &&
       comp->value()->CompileTypeIsMoreSpecificThan(comp->dst_type())) {
+    // TODO(regis): Remove is_eliminated_ field and support.
     comp->eliminate();
+    if (is_ssa_) {
+      UseVal* use = comp->value()->AsUse();
+      // TODO(regis): Handle constant input value (not a definition).

[srdjan, 2012/08/15 17:49:56]

s/not a definition/cannot access definition/

[regis, 2012/08/15 22:49:18]

Done.

+      if (use != NULL) {
+        Definition* result = use->definition();
+        ASSERT(result != NULL);
+        // Replace uses and remove the current instructions via the iterator.
+        instr->ReplaceUsesWith(result);
+        ASSERT(current_iterator()->Current()->AsBind() == instr);
+        current_iterator()->RemoveCurrentFromGraph();
+        if (FLAG_trace_optimization) {
+          OS::Print("Replacing v%d with v%d\n",
+                    instr->ssa_temp_index(),
+                    result->ssa_temp_index());

[srdjan, 2012/08/15 17:49:56]

You are not really replacing, but removing. Please adapt output.

[regis, 2012/08/15 22:49:18]

I have removed the instruction, but replaced its uses. I think the text is
correct.

+        }
+      }
+    }
     if (FLAG_trace_type_check_elimination) {
       FlowGraphPrinter::PrintTypeCheck(parsed_function(),
                                        comp->token_pos(),
@@ -613,11 +630,37 @@
 
 void FlowGraphTypePropagator::VisitAssertBoolean(AssertBooleanComp* comp,
                                                  BindInstr* instr) {
+  // TODO(regis): Propagate NullType as well and revise the comment and code
+  // below to also eliminate the test for non-null and non-constant value.
+
+  // We can only eliminate an 'assert boolean' test when the checked value is
+  // a constant time constant. Indeed, a variable of the proper compile time
+  // type (bool) may still hold null at run time and therefore fail the test.
   if (FLAG_eliminate_type_checks &&
       !comp->is_eliminated() &&
+      comp->value()->IsConstant() &&
+      !comp->value()->IsConstantNull() &&
       comp->value()->CompileTypeIsMoreSpecificThan(
           Type::Handle(Type::BoolInterface()))) {
+    // TODO(regis): Remove is_eliminated_ field and support.

[srdjan, 2012/08/15 17:49:56]

I do not think this is correct. Let's discuss offline before submitting.

[srdjan, 2012/08/15 19:55:56]

My mistake. You are doing it only for constant input values. Please verify that
this is ever hit. We may refer to a constant often via a UseVal->
BindInstr->ConstantVal

[regis, 2012/08/15 22:49:18]

Yes, we only do this for constants, but this will change.
You are correct that constants are never used directly but via a UseVal and
Bind. I have therefore removed the check and replaced  it with an assert here
and in other similar optimizations.

     comp->eliminate();
+    if (is_ssa_) {
+      UseVal* use = comp->value()->AsUse();
+      // TODO(regis): Handle constant input value (not a definition).
+      if (use != NULL) {
+        Definition* result = use->definition();
+        ASSERT(result != NULL);
+        // Replace uses and remove the current instructions via the iterator.
+        instr->ReplaceUsesWith(result);
+        ASSERT(current_iterator()->Current()->AsBind() == instr);
+        current_iterator()->RemoveCurrentFromGraph();
+        if (FLAG_trace_optimization) {
+          OS::Print("Replacing v%d with v%d\n",
+                    instr->ssa_temp_index(),
+                    result->ssa_temp_index());
+        }
+      }
+    }
     if (FLAG_trace_type_check_elimination) {
       const String& name = String::Handle(Symbols::New("boolean expression"));
       FlowGraphPrinter::PrintTypeCheck(parsed_function(),
@@ -631,6 +674,50 @@
 }
 
 
+void FlowGraphTypePropagator::VisitInstanceOf(InstanceOfComp* comp,
+                                              BindInstr* instr) {
+  // TODO(regis): Propagate NullType as well and revise the comment and code
+  // below to also eliminate the test for non-null and non-constant value.
+
+  // We can only eliminate an 'instance of' test when the checked value is
+  // a constant time constant. Indeed, a variable of the proper compile time
+  // type may still hold null at run time and therefore fail the test.
+  // We do not bother checking for Object destination type, since the graph
+  // builder did already.
+  if (FLAG_eliminate_type_checks &&
+      comp->value()->IsConstant() &&
+      !comp->value()->IsConstantNull() &&
+      comp->value()->CompileTypeIsMoreSpecificThan(comp->type())) {

[srdjan, 2012/08/15 17:49:56]

ditto

+    if (is_ssa_) {
+      UseVal* use = comp->value()->AsUse();
+      // TODO(regis): Handle constant input value (not a definition).
+      if (use != NULL) {
+        Definition* result = use->definition();
+        ASSERT(result != NULL);
+        // Replace uses and remove the current instructions via the iterator.
+        instr->ReplaceUsesWith(result);
+        ASSERT(current_iterator()->Current()->AsBind() == instr);
+        current_iterator()->RemoveCurrentFromGraph();
+        if (FLAG_trace_optimization) {
+          OS::Print("Replacing v%d with v%d\n",
+                    instr->ssa_temp_index(),
+                    result->ssa_temp_index());
+        }
+      }
+    }
+    if (FLAG_trace_type_check_elimination) {
+      const String& name = String::Handle(Symbols::New("InstanceOf"));
+      FlowGraphPrinter::PrintTypeCheck(parsed_function(),
+                                       comp->token_pos(),
+                                       comp->value(),
+                                       comp->type(),
+                                       name,
+                                       /* eliminated = */ true);
+    }
+  }
+}
+
+
 void FlowGraphTypePropagator::VisitGraphEntry(GraphEntryInstr* graph_entry) {
   if (graph_entry->start_env() == NULL) {
     return;
@@ -666,11 +753,16 @@
   // PhiInstr's are handled as part of JoinEntryInstr.
   // Visit computation and possibly eliminate type check.
   bind->computation()->Accept(this, bind);
-  // Cache propagated computation type.
-  AbstractType& type = AbstractType::Handle(bind->computation()->CompileType());
-  bool changed = bind->SetPropagatedType(type);
-  if (changed) {
-    still_changing_ = true;
+  // The current bind may have been removed from the graph.
+  if (current_iterator()->Current()->AsBind() == bind) {
+    // Current bind was not removed.
+    // Cache propagated computation type.
+    AbstractType& computation_type =
+        AbstractType::Handle(bind->computation()->CompileType());
+    bool changed = bind->SetPropagatedType(computation_type);
+    if (changed) {
+      still_changing_ = true;
+    }
   }
 }