Constant fold strict comparison based on incoming types: different (exact) types means that the res…

Constant fold strict comparison based on incoming types: different (exact) types means that the result is false. LoadStaticField computes type for final fields. Make a temporary buffer final.

R=kmillikin@google.com

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

[Kevin Millikin (Google), 2013-11-07 11:29:37]

Sorry for the delay, Srdjan.

This change inadvertently disables folding for most of the non-special cases of
equality compare.  Two fixes are suggested below.

It should be the case that 'var x = 21; identical(42, x + x)' still gets folded.

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

https://codereview.chromium.org/57703004/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:6310: return;
If you want the early return, this should be:

if ((left.IsNull() && instr->right()->Type()->HasDecidableNullability()) ||
    (right.IsNull() && instr->left()->Type()->HasDecidableNullability())) {
  // ...
} else {
  SetValue(instr, non_constant_);
}
return;

https://codereview.chromium.org/57703004/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:6330: SetValue(instr, non_constant_);
Abbreviate non-constant by X, constant by C, unknown constant by ?.  We don't
want to transition to non_constant in the cases [C,?], [?,C], and [?,?].

We only want to make this transition if one of them is X (which is why I suggest
catching it above in the case for one input non-constant).  We can remove this
call to SetValue if we have the one above.

An alternative fix is to instead have have here:

if (!IsUnknown(left) && !IsUnknown(right)) {
  SetValue(instr, non_constant_);
}

but I think that's less clear.

[srdjan, 2013-11-07 23:30:34]

Thank you for noticing the mistake. The original structure was actually quite
good. Just before we decide that the result is non-constant I now check for the
types as well. Seems like the simplest way to add the change.

[Kevin Millikin (Google), 2013-11-08 10:32:15]

LGTM Srdjan.

Thinking about it a bit more, in practice we might not have been able to get
into the situation I imagined.

That's due to the order that we add and remove blocks and instructions from the
worklist.  But we definitely don't want to rely on that here so your fix is
good.

[srdjan, 2013-11-08 17:42:52]

Committed patchset #3 manually as r30119 (presubmit successful).