Restoring the remaining CL 104893003. For instance call representing numerical comparisons and bina…

Restoring the remaining CL 104893003. For instance call representing numerical comparisons and binary operations (double and Smi supported for now) that were never visited before optimization occurs, we guess the cids if one of the argument cids is known, e.g. ‘a == 1’ we guess that a is also going to be 1 thus avoiding emitting an IC-call in optimized code and propagating the type accordingly.

Remove AssertAssignableInstr::RecomputeType as it may cause incorrect behavior: it changes the type of its value based on input, the graph below will then incorrectly remove CheckSmi.

v27 <- BinaryMintOp:14(..

v28 <- AssertAssignable(v27)
CheckSmi(v28)
v17 <- BinarySmiOp:86(+, v28, v12)

R=fschneider@google.com

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

[Florian Schneider, 2013-12-13 13:11:09]

lgtm

https://codereview.chromium.org/114543004/diff/90001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (left):

https://codereview.chromium.org/114543004/diff/90001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:773: bool
AssertAssignableInstr::RecomputeType() {
Do you have a regression test for this? I don't understand yet why this is
problematic: Changing the type based on inputs in not incorrect per se:
Phi-instructions do the same behavior.

Usually, the compile-type should not have any impact on smi-checks, only
class-id propagation is used for smi-check elimination.

Maybe strengthening of asserts is broken? I would expect the following to
happen: The AssertAssignable should be _replaced_ with a preceding checksmi.
This newly inserted CheckSmi makes the original CheckSmi redundant.

[srdjan, 2013-12-13 17:08:51]

https://codereview.chromium.org/114543004/diff/90001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (left):

https://codereview.chromium.org/114543004/diff/90001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:773: bool
AssertAssignableInstr::RecomputeType() {
On 2013/12/13 13:11:09, Florian Schneider wrote:
> Do you have a regression test for this? I don't understand yet why this is
> problematic: Changing the type based on inputs in not incorrect per se:
> Phi-instructions do the same behavior.
> 
> Usually, the compile-type should not have any impact on smi-checks, only
> class-id propagation is used for smi-check elimination.
> 
> Maybe strengthening of asserts is broken? I would expect the following to
> happen: The AssertAssignable should be _replaced_ with a preceding checksmi.
> This newly inserted CheckSmi makes the original CheckSmi redundant.
> 

Yes added regression. My initial analysis is that the problem with
AssertAssignable is that its type is the same as of its input (e.g., a binary
operation), thus changing it we modify the type of that binary operation as
well.

[srdjan, 2013-12-13 17:15:10]

Committed patchset #8 manually as r31137 (presubmit successful).