Convert some compiler passes to preserve valid def-use chains.

Convert some compiler passes to preserve valid def-use chains.

Change the ApplyICData, ApplyClassIds, Canonicalize, and
SelectRepresentations passes to preserve valid use lists.

Committed: https://code.google.com/p/dart/source/detail?r=18676

[Vyacheslav Egorov (Google), 2013-02-08 16:44:49]

LGTM with comments addressed (especially the one about
ForwardInstructionIterator::ReplaceCurrentWith)

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph.cc#newc...
runtime/vm/flow_graph.cc:213: (instr->previous() != NULL));
Consider changing WasEliminated() and using it:

bool WasEliminated() const {
  ASSERT(!IsGraphEntry());
  return !IsPhi() && instr->previous();
}

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph.cc#newc...
runtime/vm/flow_graph.cc:228: (instr->previous() != NULL));
ditto

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:248: boxed->AddInputUse(use);
I don't think these three lines are needed. You can remove them.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:547: Definition** index) {
it seems the index is not assigned in the body any more. 

you can convert it from Definition** to Definition*

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1724: instr->set_env(NULL);
Loop + set_env(NULL) can be factored out into a helper (used in two places).

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1735: Instruction* instr =
current_iterator()->Current();
This is somewhat confusing. 

Consider calling HandleRelationalOp as HandleCurrentRelationalOp or something
else to indicate that it depends on the current position of the optimizer.

Or better yet, still pass branch into it.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1770: T current_instruction) {
I think this function is un-necessarily a template.

I also find BranchInstr::ReplaceWith method extremely confusing because it does
something that I would not expect from knowing Definition::ReplaceWith.

My suggestion here would be to remove templating of this function and make this
function return a replacement.

Then caller can handle replacement with either Definition::ReplaceWith or
BranchInstr::ReplaceComparisonWith.

It is also true that this is not in sync with HandleRelationalOp in terms of
passing in the current instruction.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:2330: RenameDominatedUses(defn, constraint,
constraint);
That was an easy fix. Nice!

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (left):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:380: Definition* defn =
current_->AsDefinition();
This must come back before you will land it.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:373: inline Definition* ArgumentAt(intptr_t
index) const;
I like the naming cleanup.

[Kevin Millikin (Google), 2013-02-19 10:49:30]

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph.cc#newc...
runtime/vm/flow_graph.cc:213: (instr->previous() != NULL));
On 2013/02/08 16:44:49, Vyacheslav Egorov (Google) wrote:
> Consider changing WasEliminated() and using it:
> 
> bool WasEliminated() const {
>   ASSERT(!IsGraphEntry());
>   return !IsPhi() && instr->previous();
> }

I think it's better to (a) remove phis eagerly and (b) not have comparison as an
instruction (it can only be part of a BranchInstr or a "BoolifyInstr").

As it is, WasEliminated above just gives false negatives for phis and false
positives for comparisons.  That seems misleading to expose as a general
predicate blessed by an API, so I'd rather just keep the verification low level.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:248: boxed->AddInputUse(use);
On 2013/02/08 16:44:49, Vyacheslav Egorov (Google) wrote:
> I don't think these three lines are needed. You can remove them.

You are right.  Thank you.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1770: T current_instruction) {
I agree the architecture is a mess.  It's a preexisting mess that is, as you
say, confusing.  I'd rather not change it as part of this effort.

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (left):

https://codereview.chromium.org/12212093/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:380: Definition* defn =
current_->AsDefinition();
On 2013/02/08 16:44:49, Vyacheslav Egorov (Google) wrote:
> This must come back before you will land it.

Thanks, it was an inadvertent deletion.