Fix a bug in SSA renaming.

Fix a bug in SSA renaming.

This CL changes the order of step 2a and 2b in the SSA renaming: Uses of
LoadLocal/StoreLocal must be renamed first so that the environment is correctly
updated for StoreLocal instructions.

A simple example showing the bug:

test_xy() {
  var x = 1;
  var y = 2;
  var z = 3;
  for (var i = 0; i < 5; i++) {
    y = z = x;
    x++;
  }
  return x + y + z;
}

Also in this change:
- changed the printing of SSA operands to v0, v1, .. instead
of t0, t1 to better distinguish them from non-SSA operands.
- make sure that UseVal are always copied when renaming phi 
input operands. 
Committed: https://code.google.com/p/dart/source/detail?r=8893

[Florian Schneider, 2012-06-19 13:50:45]

Adding srdjan.

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

LGTM

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc
File vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2646: // enviroment.
s/enviroment/environment/

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2677: if ((load != NULL) || (store != NULL)) {
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.

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2715: // Rename input operand and make a copy if it is
a UseVal.
Indent comment

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

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc
File vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2646: // enviroment.
On 2012/06/19 16:45:06, srdjan wrote:
> s/enviroment/environment/

Done.

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2677: if ((load != NULL) || (store != NULL)) {
On 2012/06/19 16:45:06, srdjan wrote:
> 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.

Done.

http://codereview.chromium.org/10583014/diff/1/vm/flow_graph_builder.cc#newco...
vm/flow_graph_builder.cc:2715: // Rename input operand and make a copy if it is
a UseVal.
On 2012/06/19 16:45:06, srdjan wrote:
> Indent comment

Done.