Fix a bug in SSA renaming.

vm/flow_graph_builder.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_builder.h"
 
 #include "vm/ast_printer.h"
 #include "vm/bit_vector.h"
 #include "vm/code_descriptors.h"
 #include "vm/dart_entry.h"
@@ skipping 2586 matching lines @@
     if (join_block->PredecessorAt(i) == predecessor) return i;
   }
   UNREACHABLE();
   return -1;
 }
 
 
 void FlowGraphBuilder::RenameRecursive(BlockEntryInstr* block_entry,
                                        ZoneGrowableArray<Value*>* env,
                                        intptr_t var_count) {
-  // Iterate over instructions.
-  // 1. Handle phis first.
+  // 1. Process phis first.
   if (block_entry->IsJoinEntry()) {
     JoinEntryInstr* join = block_entry->AsJoinEntry();
     if (join->phis() != NULL) {
       for (intptr_t i = 0; i < join->phis()->length(); ++i) {
         PhiInstr* phi = (*join->phis())[i];
         if (phi != NULL) {
           (*env)[i] = new UseVal(phi);
           phi->set_ssa_temp_index(current_ssa_temp_index_++);  // New SSA temp.
         }
       }
     }
   }
 
-  // 2. Handle normal instructions.
+  // 2. Process normal instructions.
   Instruction* current = block_entry->StraightLineSuccessor();
   Instruction* prev = block_entry;
   while ((current != NULL) && !current->IsBlockEntry()) {
-    // 2a. Handle LoadLocal and StoreLocal.
-    // LoadLocal should not be present in an effect context.
-    ASSERT(!current->IsDo() ||
-           !current->AsDo()->computation()->IsLoadLocal());
-    LoadLocalComp* load = NULL;
-    if (current->IsBind() &&
-               current->AsBind()->computation()->IsLoadLocal()) {
-      load = current->AsBind()->computation()->AsLoadLocal();
-    }
-    StoreLocalComp* store = NULL;
-    if (current->IsDo() &&
-        current->AsDo()->computation()->IsStoreLocal()) {
-      store = current->AsDo()->computation()->AsStoreLocal();
-    } else if (current->IsBind() &&
-               current->AsBind()->computation()->IsStoreLocal()) {
-      store = current->AsBind()->computation()->AsStoreLocal();
-    }
-
-    if ((load != NULL) || (store != NULL)) {
-      // Remove instruction with LoadLocal or StoreLocal.
-      prev->SetSuccessor(current->StraightLineSuccessor());
-      // Update renaming environment for StoreLocal.
-      if (store != NULL) {
-        (*env)[store->local().BitIndexIn(var_count)] = store->value();
-      }
-    } else {
-      // Assign new SSA temporary.
-      if (current->IsBind()) {
-        current->AsDefinition()->set_ssa_temp_index(current_ssa_temp_index_++);
-      }
-    }
-
-    // 2b. Handle uses of LoadLocal / StoreLocal
+    // 2a. Handle uses of LoadLocal / StoreLocal
+    // For each use of a LoadLocal or StoreLocal: Replace it with the value
+    // from the environment.
     for (intptr_t i = 0; i < current->InputCount(); ++i) {
-      // For each use of a LoadLocal/StoreLocal: Replace it with the definition
-      // from the environment.
       Value* v = current->InputAt(i);
       if (v->IsUse() &&
           v->AsUse()->definition()->IsBind() &&
           v->AsUse()->definition()->AsBind()->computation()->IsLoadLocal()) {
         Computation* comp = v->AsUse()->definition()->AsBind()->computation();
         intptr_t index = comp->AsLoadLocal()->local().BitIndexIn(var_count);
         Value* new_value = (*env)[index];
         // Make a copy if it is a UseVal.
         if (new_value->IsUse()) {
           new_value = new UseVal(new_value->AsUse()->definition());
         }
         current->SetInputAt(i, new_value);
       }
       if (v->IsUse() &&
           v->AsUse()->definition()->IsBind() &&
           v->AsUse()->definition()->AsBind()->computation()->IsStoreLocal()) {
-        // For each use of a LoadLocal: Replace LoadLocal with the definition
-        // from the enviroment.
+        // For each use of a StoreLocal: Replace it with the value from the
+        // enviroment.

[srdjan, 2012/06/19 16:45:06]

s/enviroment/environment/

[Florian Schneider, 2012/06/21 08:15:47]

Done.

         Computation* comp = v->AsUse()->definition()->AsBind()->computation();
         intptr_t index = comp->AsStoreLocal()->local().BitIndexIn(var_count);
         Value* new_value = (*env)[index];
         // Make a copy if it is a UseVal.
         if (new_value->IsUse()) {
           new_value = new UseVal(new_value->AsUse()->definition());
         }
         current->SetInputAt(i, new_value);
       }
     }
 
-    // Update previous only if no instruction was removed from the graph.
-    if ((load == NULL) && (store == NULL)) {
+    // 2b. Handle LoadLocal and StoreLocal.
+    // For each LoadLocal: Remove it from the graph.
+    // For each StoreLocal: Remove it from the graph and update the environment.
+    ASSERT(!current->IsDo() ||
+           !current->AsDo()->computation()->IsLoadLocal());  // Not possible.
+    LoadLocalComp* load = NULL;
+    if (current->IsBind() &&
+               current->AsBind()->computation()->IsLoadLocal()) {
+      load = current->AsBind()->computation()->AsLoadLocal();
+    }
+    StoreLocalComp* store = NULL;
+    if (current->IsDo() &&
+        current->AsDo()->computation()->IsStoreLocal()) {
+      store = current->AsDo()->computation()->AsStoreLocal();
+    } else if (current->IsBind() &&
+               current->AsBind()->computation()->IsStoreLocal()) {
+      store = current->AsBind()->computation()->AsStoreLocal();
+    }
+
+    if ((load != NULL) || (store != NULL)) {

[srdjan, 2012/06/19 16:45:06]

I think it would be more readable if you split it (since it is not possible that
both store and load are not NULL:

if (load != NULL) {
  // Remove instruction with LoadLocal
  prev->SetSuccessor(current->StraightLineSuccessor());
} else if (store != NULL) {
  // Remove instruction with StoreLocal
  prev->SetSuccessor(current->StraightLineSuccessor());
  (*env)[store->local().BitIndexIn(var_count)] = store->value();
} else {
  // Assign new SSA temporary.
...
}

You may (or not)  merge the functionality with if-s above.

[Florian Schneider, 2012/06/21 08:15:47]

Done.

+      // Remove instruction with LoadLocal or StoreLocal.
+      prev->SetSuccessor(current->StraightLineSuccessor());
+      // Update renaming environment for StoreLocal.
+      if (store != NULL) {
+        (*env)[store->local().BitIndexIn(var_count)] = store->value();
+      }
+    } else {
+      // Assign new SSA temporary.
+      if (current->IsBind()) {
+        current->AsDefinition()->set_ssa_temp_index(current_ssa_temp_index_++);
+      }
+      // Update previous only if no instruction was removed from the graph.
       prev = current;
     }
     current = current->StraightLineSuccessor();
   }
 
   // 3. Process dominated blocks.
   for (intptr_t i = 0; i < block_entry->dominated_blocks().length(); ++i) {
     BlockEntryInstr* block = block_entry->dominated_blocks()[i];
     ZoneGrowableArray<Value*>* new_env =
         new ZoneGrowableArray<Value*>(var_count);
     new_env->AddArray(*env);
     RenameRecursive(block, new_env, var_count);
   }
 
   // 4. Process successor block. We have edge-split form, so that only blocks
   // with one successor can have a join block as successor.
   if ((block_entry->last_instruction()->SuccessorCount() == 1) &&
       block_entry->last_instruction()->SuccessorAt(0)->IsJoinEntry()) {
     JoinEntryInstr* successor =
         block_entry->last_instruction()->SuccessorAt(0)->AsJoinEntry();
     intptr_t pred_index = WhichPred(block_entry, successor);
     if (successor->phis() != NULL) {
       for (intptr_t i = 0; i < successor->phis()->length(); ++i) {
         PhiInstr* phi = (*successor->phis())[i];
-        if (phi != NULL) phi->SetInputAt(pred_index, (*env)[i]);
+        if (phi != NULL) {
+        // Rename input operand and make a copy if it is a UseVal.

[srdjan, 2012/06/19 16:45:06]

Indent comment

[Florian Schneider, 2012/06/21 08:15:47]

Done.

+          Value* new_val = (*env)[i]->IsUse()
+              ? new UseVal((*env)[i]->AsUse()->definition())
+              : (*env)[i];
+          phi->SetInputAt(pred_index, new_val);
+        }
       }
     }
   }
 }
 
 
 void FlowGraphBuilder::Bailout(const char* reason) {
   const char* kFormat = "FlowGraphBuilder Bailout: %s %s";
   const char* function_name = parsed_function_.function().ToCString();
   intptr_t len = OS::SNPrint(NULL, 0, kFormat, function_name, reason) + 1;
   char* chars = reinterpret_cast<char*>(
       Isolate::Current()->current_zone()->Allocate(len));
   OS::SNPrint(chars, len, kFormat, function_name, reason);
   const Error& error = Error::Handle(
       LanguageError::New(String::Handle(String::New(chars))));
   Isolate::Current()->long_jump_base()->Jump(1, error);
 }
 
 
 }  // namespace dart