Refactor building of StoreStaticField and StoreLocal to manually preserve value.

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
diff --git a/runtime/vm/flow_graph_builder.cc b/runtime/vm/flow_graph_builder.cc
index 210a149e48d50ff7d42067ffcb28399c28e00351..7ebe3b0a449883794d8132e7f3ea3b7cd6b56a8a 100644
--- a/runtime/vm/flow_graph_builder.cc
+++ b/runtime/vm/flow_graph_builder.cc
@@ -209,8 +209,15 @@ PushArgumentInstr* EffectGraphVisitor::PushArgument(Value* value) {
 
 
 Definition* EffectGraphVisitor::BuildStoreLocal(
-    const LocalVariable& local, Value* value) {
+    const LocalVariable& local, Value* value, bool result_is_needed) {
   if (local.is_captured()) {
+    if (result_is_needed) {
+      value = Bind(
+          BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
+                          value,
+                          kResultNeeded));
+    }
+
     intptr_t delta =
         owner()->context_level() - local.owner()->context_level();
     ASSERT(delta >= 0);
@@ -219,11 +226,18 @@ Definition* EffectGraphVisitor::BuildStoreLocal(
       context = Bind(new LoadVMFieldInstr(
           context, Context::parent_offset(), Type::ZoneHandle()));
     }
-    return new StoreVMFieldInstr(
-        context,
-        Context::variable_offset(local.index()),
-        value,
-        local.type());
+
+    StoreVMFieldInstr* store =
+        new StoreVMFieldInstr(context,
+                              Context::variable_offset(local.index()),
+                              value,
+                              local.type());
+    if (result_is_needed) {
+      Do(store);
+      return BuildLoadLocal(*owner()->parsed_function().expression_temp_var());
+    } else {
+      return store;
+    }
   } else {
     return new StoreLocalInstr(local, value, owner()->context_level());
   }
@@ -252,7 +266,7 @@ Definition* EffectGraphVisitor::BuildLoadLocal(const LocalVariable& local) {
 // Stores current context into the 'variable'
 void EffectGraphVisitor::BuildStoreContext(const LocalVariable& variable) {
   Value* context = Bind(new CurrentContextInstr());
-  Do(BuildStoreLocal(variable, context));
+  Do(BuildStoreLocal(variable, context, kResultNotNeeded));
 }
 
 
@@ -658,14 +672,16 @@ void ValueGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
                                               constant_true));
     for_right.Do(BuildStoreLocal(
         *owner()->parsed_function().expression_temp_var(),
-        compare));
+        compare,
+        kResultNotNeeded));

[Florian Schneider, 2012/09/10 14:38:21]

The parameter result_not_needed is not needed when storing to expr_temp since it
is never captured.

[Vyacheslav Egorov (Google), 2012/09/10 16:00:14]

Done.

 
     if (node->kind() == Token::kAND) {
       ValueGraphVisitor for_false(owner(), temp_index());
       Value* constant_false = for_false.Bind(new ConstantInstr(bool_false));
       for_false.Do(BuildStoreLocal(
           *owner()->parsed_function().expression_temp_var(),
-          constant_false));
+          constant_false,
+          kResultNotNeeded));
       Join(for_test, for_right, for_false);
     } else {
       ASSERT(node->kind() == Token::kOR);
@@ -673,7 +689,8 @@ void ValueGraphVisitor::VisitBinaryOpNode(BinaryOpNode* node) {
       Value* constant_true = for_true.Bind(new ConstantInstr(bool_true));
       for_true.Do(BuildStoreLocal(
           *owner()->parsed_function().expression_temp_var(),
-          constant_true));
+          constant_true,
+          kResultNotNeeded));
       Join(for_test, for_true, for_right);
     }
     ReturnDefinition(
@@ -705,7 +722,9 @@ void EffectGraphVisitor::BuildTypecheckArguments(
     // Preserve instantiator.
     const LocalVariable& expr_temp =
         *owner()->parsed_function().expression_temp_var();
-    instantiator = Bind(BuildStoreLocal(expr_temp, instantiator));
+    instantiator = Bind(BuildStoreLocal(expr_temp,
+                                        instantiator,
+                                        kResultNotNeeded));
     Value* loaded = Bind(BuildLoadLocal(expr_temp));
     instantiator_type_arguments =
         BuildInstantiatorTypeArguments(token_pos, loaded);
@@ -1008,13 +1027,17 @@ void ValueGraphVisitor::VisitConditionalExprNode(ConditionalExprNode* node) {
   node->true_expr()->Visit(&for_true);
   ASSERT(for_true.is_open());
   for_true.Do(BuildStoreLocal(
-      *owner()->parsed_function().expression_temp_var(), for_true.value()));
+      *owner()->parsed_function().expression_temp_var(),
+      for_true.value(),
+      kResultNotNeeded));
 
   ValueGraphVisitor for_false(owner(), temp_index());
   node->false_expr()->Visit(&for_false);
   ASSERT(for_false.is_open());
   for_false.Do(BuildStoreLocal(
-      *owner()->parsed_function().expression_temp_var(), for_false.value()));
+      *owner()->parsed_function().expression_temp_var(),
+      for_false.value(),
+      kResultNotNeeded));
 
   Join(for_test, for_true, for_false);
   ReturnDefinition(
@@ -1813,7 +1836,7 @@ void EffectGraphVisitor::BuildConstructorTypeArguments(
   Value* instantiator_type_arguments = BuildInstantiatorTypeArguments(
       node->token_pos(), NULL);
   Value* stored_instantiator =
-      Bind(BuildStoreLocal(t1, instantiator_type_arguments));
+      Bind(BuildStoreLocal(t1, instantiator_type_arguments, kResultNeeded));
   // t1: instantiator type arguments.
 
   Value* extract_type_arguments = Bind(
@@ -1822,13 +1845,13 @@ void EffectGraphVisitor::BuildConstructorTypeArguments(
           node->type_arguments(),
           stored_instantiator));
 
-  Do(BuildStoreLocal(t2, extract_type_arguments));
+  Do(BuildStoreLocal(t2, extract_type_arguments, kResultNotNeeded));
   // t2: extracted constructor type arguments.
   Value* load_instantiator = Bind(BuildLoadLocal(t1));
 
   Value* extract_instantiator =
       Bind(new ExtractConstructorInstantiatorInstr(node, load_instantiator));
-  Do(BuildStoreLocal(t1, extract_instantiator));
+  Do(BuildStoreLocal(t1, extract_instantiator, kResultNotNeeded));
   // t2: extracted constructor type arguments.
   // t1: extracted constructor instantiator.
   Value* type_arguments_val = Bind(BuildLoadLocal(t2));
@@ -1869,7 +1892,8 @@ void ValueGraphVisitor::VisitConstructorCallNode(ConstructorCallNode* node) {
   Value* allocate = BuildObjectAllocation(node);
   Definition* store_allocated = BuildStoreLocal(
       node->allocated_object_var(),
-      allocate);
+      allocate,
+      kResultNeeded);
   Value* allocated_value = Bind(store_allocated);

[Florian Schneider, 2012/09/10 14:38:21]

Maybe rephrase this for consistency:

Value* allocated_value = Bind(BuildStoreLocal(node->allocated_object_var(),
allocate, kResultNeeded);

[Vyacheslav Egorov (Google), 2012/09/10 16:00:14]

Done.

   PushArgumentInstr* push_allocated_value = PushArgument(allocated_value);
   BuildConstructorCall(node, push_allocated_value);
@@ -1916,7 +1940,8 @@ void EffectGraphVisitor::BuildInstanceSetterArguments(
   if (result_is_needed) {
     value = Bind(
         BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
-                        for_value.value()));
+                        for_value.value(),
+                        kResultNeeded));
   } else {
     value = for_value.value();
   }
@@ -1928,7 +1953,7 @@ void EffectGraphVisitor::VisitInstanceSetterNode(InstanceSetterNode* node) {
   InlineBailout("EffectGraphVisitor::VisitInstanceSetterNode");
   ZoneGrowableArray<PushArgumentInstr*>* arguments =
       new ZoneGrowableArray<PushArgumentInstr*>(2);
-  BuildInstanceSetterArguments(node, arguments, false);  // Value not used.
+  BuildInstanceSetterArguments(node, arguments, kResultNotNeeded);
   const String& name =
       String::ZoneHandle(Field::SetterSymbol(node->field_name()));
   InstanceCallInstr* call = new InstanceCallInstr(node->token_pos(),
@@ -1945,7 +1970,7 @@ void ValueGraphVisitor::VisitInstanceSetterNode(InstanceSetterNode* node) {
   InlineBailout("ValueGraphVisitor::VisitInstanceSetterNode");
   ZoneGrowableArray<PushArgumentInstr*>* arguments =
       new ZoneGrowableArray<PushArgumentInstr*>(2);
-  BuildInstanceSetterArguments(node, arguments, true);  // Value used.
+  BuildInstanceSetterArguments(node, arguments, kResultNeeded);
   const String& name =
       String::ZoneHandle(Field::SetterSymbol(node->field_name()));
   Do(new InstanceCallInstr(node->token_pos(),
@@ -2019,7 +2044,8 @@ void EffectGraphVisitor::BuildStaticSetter(StaticSetterNode* node,
   if (result_is_needed) {
     value = Bind(
         BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
-                        for_value.value()));
+                        for_value.value(),
+                        kResultNeeded));
   } else {
     value = for_value.value();
   }
@@ -2086,7 +2112,8 @@ void ValueGraphVisitor::VisitLoadLocalNode(LoadLocalNode* node) {
 
 // <Expression> ::= StoreLocal { local: LocalVariable
 //                               value: <Expression> }
-void EffectGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
+void EffectGraphVisitor::VisitStoreLocal(StoreLocalNode* node,

[Florian Schneider, 2012/09/10 14:38:21]

Maybe HandleStoreLocal, otherwise it looks like a visitor function.

[Vyacheslav Egorov (Google), 2012/09/10 16:00:14]

Done.

+                                         bool result_is_needed) {
   InlineBailout("EffectGraphVisitor::VisitStoreLocalNode");
   ValueGraphVisitor for_value(owner(), temp_index());
   node->value()->Visit(&for_value);
@@ -2098,11 +2125,23 @@ void EffectGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
                                        node->local().type(),
                                        node->local().name());
   }
-  Definition* store = BuildStoreLocal(node->local(), store_value);
+  Definition* store = BuildStoreLocal(node->local(),
+                                      store_value,
+                                      result_is_needed);
   ReturnDefinition(store);
 }
 
 
+void EffectGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
+  VisitStoreLocal(node, kResultNotNeeded);
+}
+
+
+void ValueGraphVisitor::VisitStoreLocalNode(StoreLocalNode* node) {
+  VisitStoreLocal(node, kResultNeeded);
+}
+
+
 void EffectGraphVisitor::VisitLoadInstanceFieldNode(
     LoadInstanceFieldNode* node) {
   InlineBailout("EffectGraphVisitor::VisitLoadInstanceFieldNode");
@@ -2154,12 +2193,21 @@ void EffectGraphVisitor::VisitLoadStaticFieldNode(LoadStaticFieldNode* node) {
 }
 
 
-void EffectGraphVisitor::VisitStoreStaticFieldNode(StoreStaticFieldNode* node) {
+Definition* EffectGraphVisitor::BuildStoreStaticField(
+  StoreStaticFieldNode* node, bool result_is_needed) {
   InlineBailout("EffectGraphVisitor::VisitStoreStaticFieldNode");
   ValueGraphVisitor for_value(owner(), temp_index());
   node->value()->Visit(&for_value);
   Append(for_value);
-  Value* store_value = for_value.value();
+  Value* store_value = NULL;
+  if (result_is_needed) {
+    store_value = Bind(
+        BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),

[Florian Schneider, 2012/09/10 14:38:21]

I'd have a separate helper to avoid confusion:

 BuildStoreTempLocal(const LocalVariable& temp, Value* value)  and BuildLoadTemp
for storing/loading into temporary locals (they are never captured, and
therefore don't need the result_is_needed parameter).

[Vyacheslav Egorov (Google), 2012/09/10 16:00:14]

Done.

+                        for_value.value(),
+                        kResultNeeded));
+  } else {
+    store_value = for_value.value();
+  }
   if (FLAG_enable_type_checks) {
     const AbstractType& type = AbstractType::ZoneHandle(node->field().type());
     const String& dst_name = String::ZoneHandle(node->field().name());
@@ -2170,7 +2218,23 @@ void EffectGraphVisitor::VisitStoreStaticFieldNode(StoreStaticFieldNode* node) {
   }
   StoreStaticFieldInstr* store =
       new StoreStaticFieldInstr(node->field(), store_value);
-  ReturnDefinition(store);
+
+  if (result_is_needed) {
+    Do(store);
+    return BuildLoadLocal(*owner()->parsed_function().expression_temp_var());
+  } else {
+    return store;
+  }
+}
+
+
+void EffectGraphVisitor::VisitStoreStaticFieldNode(StoreStaticFieldNode* node) {
+  ReturnDefinition(BuildStoreStaticField(node, kResultNotNeeded));
+}
+
+
+void ValueGraphVisitor::VisitStoreStaticFieldNode(StoreStaticFieldNode* node) {
+  ReturnDefinition(BuildStoreStaticField(node, kResultNeeded));
 }
 
 
@@ -2224,7 +2288,8 @@ Definition* EffectGraphVisitor::BuildStoreIndexedValues(
   if (result_is_needed) {
     value = Bind(
         BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
-                        for_value.value()));
+                        for_value.value(),
+                        kResultNeeded));
   } else {
     value = for_value.value();
   }
@@ -2250,15 +2315,13 @@ Definition* EffectGraphVisitor::BuildStoreIndexedValues(
 
 void EffectGraphVisitor::VisitStoreIndexedNode(StoreIndexedNode* node) {
   InlineBailout("EffectGraphVisitor::VisitStoreIndexedNode");
-  ReturnDefinition(BuildStoreIndexedValues(node,
-                                           false));  // Result not needed.
+  ReturnDefinition(BuildStoreIndexedValues(node, kResultNotNeeded));
 }
 
 
 void ValueGraphVisitor::VisitStoreIndexedNode(StoreIndexedNode* node) {
   InlineBailout("ValueGraphVisitor::VisitStoreIndexedNode");
-  ReturnDefinition(BuildStoreIndexedValues(node,
-                                           true));  // Result is needed.
+  ReturnDefinition(BuildStoreIndexedValues(node, kResultNeeded));
 }
 
 
@@ -2303,7 +2366,8 @@ void EffectGraphVisitor::VisitSequenceNode(SequenceNode* node) {
     if (MustSaveRestoreContext(node)) {
       Value* current_context = Bind(new CurrentContextInstr());
       Do(BuildStoreLocal(*owner()->parsed_function().saved_context_var(),
-                         current_context));
+                         current_context,
+                         kResultNotNeeded));
       Value* null_context = Bind(new ConstantInstr(Object::ZoneHandle()));
       Do(new StoreContextInstr(null_context));
     }
@@ -2339,13 +2403,13 @@ void EffectGraphVisitor::VisitSequenceNode(SequenceNode* node) {
 
           // Copy parameter from local frame to current context.
           Value* load = Bind(BuildLoadLocal(*temp_local));
-          Do(BuildStoreLocal(parameter, load));
+          Do(BuildStoreLocal(parameter, load, kResultNotNeeded));
           // Write NULL to the source location to detect buggy accesses and
           // allow GC of passed value if it gets overwritten by a new value in
           // the function.
           Value* null_constant =
               Bind(new ConstantInstr(Object::ZoneHandle()));
-          Do(BuildStoreLocal(*temp_local, null_constant));
+          Do(BuildStoreLocal(*temp_local, null_constant, kResultNotNeeded));
         }
       }
     }
@@ -2382,7 +2446,7 @@ void EffectGraphVisitor::VisitSequenceNode(SequenceNode* node) {
         // Store the type checked argument back to its corresponding local
         // variable so that ssa renaming detects the dependency and makes use
         // of the checked type in type propagation.
-        Do(BuildStoreLocal(parameter, parameter_value));
+        Do(BuildStoreLocal(parameter, parameter_value, kResultNotNeeded));
       }
       pos++;
     }