Cleanup handling of NullType in type propagation.

runtime/vm/flow_graph_optimizer.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/flow_graph_optimizer.h"
 
 #include "vm/flow_graph_builder.h"
 #include "vm/il_printer.h"
 #include "vm/object_store.h"
 #include "vm/parser.h"
@@ skipping 566 matching lines @@
 
 void FlowGraphOptimizer::VisitBind(BindInstr* instr) {
   instr->computation()->Accept(this, instr);
 }
 
 
 void FlowGraphTypePropagator::VisitAssertAssignable(AssertAssignableComp* comp,
                                                     BindInstr* instr) {
   if (FLAG_eliminate_type_checks &&
       !comp->is_eliminated() &&
-      !comp->dst_type().IsMalformed() &&
       comp->value()->CompileTypeIsMoreSpecificThan(comp->dst_type())) {
     comp->eliminate();
+#if 0
+    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();
+      instr->RemoveInputUses();
+      if (result->use_list() == NULL) {
+        // Remove the definition of the input as well, since it has no uses.

[Kevin Millikin (Google), 2012/08/15 09:04:25]

This is not really safe if it happens, because it could have side effects. 
E.g.:

t0 <- Call(....)
AssertAssignable(t0)
...

[regis, 2012/08/15 17:25:27]

Thanks! I removed the code removing the unused input. I was trying to understand
the assert fault I was getting. 

+        current_iterator()->RemoveCurrentFromGraph();
+      }
+
+      // TODO(regis): The above instruction removal results in an assert fault:

[Kevin Millikin (Google), 2012/08/15 09:04:25]

We should just skip this optimization if we're using the virtual frame backend. 
It is going away soon.

[regis, 2012/08/15 17:25:27]

Done. That takes care of the assert fault.

+      // ../runtime/vm/flow_graph_compiler.cc:754: error: expected:
+      // val->AsUse()->definition() == registers_[src]
+
+      if (FLAG_trace_optimization) {
+        OS::Print("Replacing v%d with v%d\n",
+                  instr->ssa_temp_index(),
+                  result->ssa_temp_index());
+      }
+    }
+#endif

[srdjan, 2012/08/15 02:07:31]

Removed dead code  before submitting

[regis, 2012/08/15 17:25:27]

The code is now enabled and working.

     if (FLAG_trace_type_check_elimination) {
       FlowGraphPrinter::PrintTypeCheck(parsed_function(),
                                        comp->token_pos(),
                                        comp->value(),
                                        comp->dst_type(),
                                        comp->dst_name(),
                                        comp->is_eliminated());
     }
   }
 }
 
 
 void FlowGraphTypePropagator::VisitAssertBoolean(AssertBooleanComp* comp,
                                                  BindInstr* instr) {
   if (FLAG_eliminate_type_checks &&
       !comp->is_eliminated() &&
+      !comp->value()->IsConstantNull() &&
       comp->value()->CompileTypeIsMoreSpecificThan(
           Type::Handle(Type::BoolInterface()))) {
+    // TODO(regis): This optimization may not be correct, since a value of
+    // compile time type bool may still be null at run time. In this case,
+    // the bool expression will evaluate to false without throwing a dynamic
+    // type error.
     comp->eliminate();
     if (FLAG_trace_type_check_elimination) {
       const String& name = String::Handle(Symbols::New("boolean expression"));
       FlowGraphPrinter::PrintTypeCheck(parsed_function(),
                                        comp->token_pos(),
                                        comp->value(),
                                        Type::Handle(Type::BoolInterface()),
                                        name,
                                        comp->is_eliminated());
     }
   }
 }
 
 
+void FlowGraphTypePropagator::VisitInstanceOf(InstanceOfComp* comp,
+                                              BindInstr* instr) {
+  // 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())) {
+    // Remove instr from graph and replace uses with use of true or false.
+    UNIMPLEMENTED();  // TODO(regis): We never encounter this case so far.
+    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);

[srdjan, 2012/08/15 02:07:31]

You cannot eliminate instance of using CompileType, and you are not calling
comp->eliminate() anyway. Remove it?

[regis, 2012/08/15 17:25:27]

I will be able to remove a non-constant value once I know if the value can be
null or not. The code is in place now.

+    }
+  }
+}
+
+
 void FlowGraphTypePropagator::VisitGraphEntry(GraphEntryInstr* graph_entry) {
   if (graph_entry->start_env() == NULL) {
     return;
   }
   // Visit incoming parameters.
   for (intptr_t i = 0; i < graph_entry->start_env()->values().length(); i++) {
     Value* val = graph_entry->start_env()->values()[i];
     if (val->IsUse()) {
       ParameterInstr* param = val->AsUse()->definition()->AsParameter();
       if (param != NULL) {
@@ skipping 15 matching lines @@
     }
   }
 }
 
 
 void FlowGraphTypePropagator::VisitBind(BindInstr* bind) {
   // No need to propagate the input types of the bound computation, as long as
   // 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.

[srdjan, 2012/08/15 02:07:31]

Can this happen, since you are not removing instructions?

[regis, 2012/08/15 17:25:27]

I am now.

+  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;
+    }
   }
 }
 
 
 void FlowGraphTypePropagator::VisitPhi(PhiInstr* phi) {
   // We could set the propagated type of the phi to the least upper bound of its
   // input propagated types. However, keeping all propagated types allows us to
   // optimize method dispatch.
   // TODO(regis): Support a set of propagated types. For now, we compute the
   // least specific of the input propagated types.
@@ skipping 46 matching lines @@
       LocationSummary* locs = it.Current()->locs();
       if ((locs != NULL) && locs->contains_call()) {
         is_leaf_ = false;
         return;
       }
     }
   }
 }
 
 }  // namespace dart