Add canonicalize optimization for branches.

Add canonicalize optimization for branches.

Comparisons where the result is only used in a branch on true
can be folded into the branch instruction.

Patterns like this can occur after inlining and constant propagation.
In normal code, we merge branches and comparisons already at graph
building time.

v3 <- (v1 == v2)
Branch if (v3 === true)

is optimized to

Branch if (v1 == v2)

Also: Remove outdated TODOs and rename the canonicalization pass to a 
better name.
Committed: https://code.google.com/p/dart/source/detail?r=16919

[Kevin Millikin (Google), 2013-01-10 10:35:00]

LGTM with a stylistic suggestion.

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

https://codereview.chromium.org/11819031/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:213: optimizer.CanonicalizeComputations();
This name is fine, but you could also call it "Canonicalize" without ambiguity.

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

https://codereview.chromium.org/11819031/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1693: Definition* replacement =
comparison()->Canonicalize(optimizer);
It might be simpler with an early return, and I like to avoid calling both IsXXX
and AsXXX on the same instruction.  Also, the code should be safe if
comparison()->Canonicalize() returns NULL (or at least assert that it doesn't). 
What do you think of:

Definition* replacement = comparison()->Canonicalize(optimizer);
if ((replacement == comparision()) || (replacement == NULL)) return this;
ComparisonInstr* comp = replacement->AsComparison();
if (comp == NULL) return this;

// Replace the comparison if the replacement is only used in this branch.
if ((comp->...) && ... && ...) ...

https://codereview.chromium.org/11819031/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1703: ReplaceWith(comp, NULL /* ignored */);
You should expand the comment about NULL so the reader doesn't have to look at
ReplaceWith.

// It's safe to pass a NULL instruction iterator because we're sure that the
replacement is not the canonicalization iterator's current instruction.

[Florian Schneider, 2013-01-10 13:18:41]

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

https://codereview.chromium.org/11819031/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:213: optimizer.CanonicalizeComputations();
On 2013/01/10 10:35:00, kmillikin wrote:
> This name is fine, but you could also call it "Canonicalize" without
ambiguity.

Done.

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

https://codereview.chromium.org/11819031/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1693: Definition* replacement =
comparison()->Canonicalize(optimizer);
On 2013/01/10 10:35:00, kmillikin wrote:
> It might be simpler with an early return, and I like to avoid calling both
IsXXX
> and AsXXX on the same instruction.  Also, the code should be safe if
> comparison()->Canonicalize() returns NULL (or at least assert that it
doesn't). 
> What do you think of:
> 
> Definition* replacement = comparison()->Canonicalize(optimizer);
> if ((replacement == comparision()) || (replacement == NULL)) return this;
> ComparisonInstr* comp = replacement->AsComparison();
> if (comp == NULL) return this;
> 
> // Replace the comparison if the replacement is only used in this branch.
> if ((comp->...) && ... && ...) ...

Done.

https://codereview.chromium.org/11819031/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:1703: ReplaceWith(comp, NULL /* ignored */);
On 2013/01/10 10:35:00, kmillikin wrote:
> You should expand the comment about NULL so the reader doesn't have to look at
> ReplaceWith.
> 
> // It's safe to pass a NULL instruction iterator because we're sure that the
> replacement is not the canonicalization iterator's current instruction.

Done.