Improve code for !identical(a, b):

Improve code for !identical(a, b):
"x = (a === b); y = (x !== true)" => "y = (a !== b)".
Committed: https://code.google.com/p/dart/source/detail?r=20550

[Kevin Millikin (Google), 2013-03-25 09:58:08]

I think we should keep all the "comparison folding" optimizations in the same
pass.  For now, that is Canonicalize, but I will eventually move them into
branch propagation because they work together with propagation.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2129: Token::kEQ_STRICT : Token::kNE_STRICT;
We should probably have a generic way to do this, either a function on the Token
class or a function on the StrictCompareInstr class.

There is a static function in this file, NegateComparison, that you can use if
you extend it to handle strict comparisons.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2134: instr->ReplaceWith(reverted_comp,
current_iterator());
I still think it's nuts that BranchInstr::ReplaceWith takes a comparison and
doesn't replace the branch with anything.  It only exists so it can be used in a
template function.

Just call instr->SetComparison directly here for clarity.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2135: left->RemoveFromGraph();
You can't remove this if it has uses, even environment ones.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2147:
HandleStrictCompare(comparison->AsStrictCompare(), instr);
I feel that this belongs better in the canonicalize pass rather than in this
pass which is doing rewriting based on types.  

There is a TODO from fschneider in StrictCompareInstr::Canonicalize to handle
exactly this pattern.  Unfortunately, it's a bit messy because you want to
rewrite two instructions.  You could try:

1. The existing pattern in StrictCompareInstr::Canonicalize, which seems a bit
warty: return the first comparison as the result of canonicalizing the second,
peek at the return value in BranchInstr::Comparison to see if the first
comparison should be removed from the graph and the second comparison replaced
with the first in the BranchInstr.  Here you can directly use the first
comparison but swap the successors of the branch.

2. When canonicalizing the first comparison, possibly eliminate it and rewrite
the second.

3. When canonicalizing the second comparison, rewrite it and  possibly eliminate
the first.

[srdjan, 2013-03-25 21:12:11]

Thanks, PTAL.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2129: Token::kEQ_STRICT : Token::kNE_STRICT;
On 2013/03/25 09:58:08, kmillikin wrote:
> We should probably have a generic way to do this, either a function on the
Token
> class or a function on the StrictCompareInstr class.
> 
> There is a static function in this file, NegateComparison, that you can use if
> you extend it to handle strict comparisons.

Extended NegateComparison and moved that functionality to Token class.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2134: instr->ReplaceWith(reverted_comp,
current_iterator());
On 2013/03/25 09:58:08, kmillikin wrote:
> I still think it's nuts that BranchInstr::ReplaceWith takes a comparison and
> doesn't replace the branch with anything.  It only exists so it can be used in
a
> template function.
> 
> Just call instr->SetComparison directly here for clarity.

Done.

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2135: left->RemoveFromGraph();
On 2013/03/25 09:58:08, kmillikin wrote:
> You can't remove this if it has uses, even environment ones.

Thanks. Adding tests to catch that. Would it be useful if we could assert that
in RemoveFromGraph? (right now we can't).

https://codereview.chromium.org/12852007/diff/4001/runtime/vm/flow_graph_opti...
runtime/vm/flow_graph_optimizer.cc:2147:
HandleStrictCompare(comparison->AsStrictCompare(), instr);
On 2013/03/25 09:58:08, kmillikin wrote:
> I feel that this belongs better in the canonicalize pass rather than in this
> pass which is doing rewriting based on types.  
> 
> There is a TODO from fschneider in StrictCompareInstr::Canonicalize to handle
> exactly this pattern.  Unfortunately, it's a bit messy because you want to
> rewrite two instructions.  You could try:
> 
> 1. The existing pattern in StrictCompareInstr::Canonicalize, which seems a bit
> warty: return the first comparison as the result of canonicalizing the second,
> peek at the return value in BranchInstr::Comparison to see if the first
> comparison should be removed from the graph and the second comparison replaced
> with the first in the BranchInstr.  Here you can directly use the first
> comparison but swap the successors of the branch.
> 
> 2. When canonicalizing the first comparison, possibly eliminate it and rewrite
> the second.
> 
> 3. When canonicalizing the second comparison, rewrite it and  possibly
eliminate
> the first.

I agree, moved.

[Kevin Millikin (Google), 2013-03-26 12:48:31]

I have a few small comments.  LGTM if you address them or choose to ignore them.

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1294: // x = (a === b);   y = x !== true; ->
y = a !== b.
Nit: extra space character in the comment.

Perhaps mention that we do not need to check left's cid (unlike just above)
because we require it to be a strict equality.

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1300:
(left_strict->input_use_list()->next_use() == NULL)) {
I suppose there should be not environment uses either.  You can write

left_strict->HasOnlyUse(left())

to check that it has only the expected use.

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1303: new StrictCompareInstr(negated_kind,
Do you think it's too unsafe to just replace the token in the existing
StrictCompareInstr?  It seems OK to me since we are removing it and have
verified it has no uses.

[srdjan, 2013-03-26 22:35:25]

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1294: // x = (a === b);   y = x !== true; ->
y = a !== b.
On 2013/03/26 12:48:31, kmillikin wrote:
> Nit: extra space character in the comment.
> 
> Perhaps mention that we do not need to check left's cid (unlike just above)
> because we require it to be a strict equality.

Done.

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1300:
(left_strict->input_use_list()->next_use() == NULL)) {
On 2013/03/26 12:48:31, kmillikin wrote:
> I suppose there should be not environment uses either.  You can write
> 
> left_strict->HasOnlyUse(left())
> 
> to check that it has only the expected use.

Thanks!

https://codereview.chromium.org/12852007/diff/17001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1303: new StrictCompareInstr(negated_kind,
On 2013/03/26 12:48:31, kmillikin wrote:
> Do you think it's too unsafe to just replace the token in the existing
> StrictCompareInstr?  It seems OK to me since we are removing it and have
> verified it has no uses.

I am on the edge on that one (tried both). Adding a kind_ field mutating
operation just for this case seems less safe than this ... on the other hand 
....

Changed as suggested.

[srdjan, 2013-03-26 23:41:34]

Committed patchset #7 manually as r20550 (presubmit successful).