Remove optimization in unoptimized code that leave the graph in bad state (type test).

Fixes crash (issue 18662).
Prevent EffectGraphVisitor from emitting constants as they have no effect and will cause SSA builder to crash. Remove one optimization in graph buidler, but left the one that eliminates type tests against dynamic and Object as that is a frequent case in checked mode and reduces generated code.

R=regis@google.com

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

[regis, 2014-05-06 23:36:56]

LGTM

Thanks for the fix!

https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:1423: ReturnDefinition(new
ConstantInstr(Bool::Get(!negate_result)));
Will this other optimization cause the same issue?

[srdjan, 2014-05-06 23:51:56]

https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:1423: ReturnDefinition(new
ConstantInstr(Bool::Get(!negate_result)));
On 2014/05/06 23:36:57, regis wrote:
> Will this other optimization cause the same issue?

Do not think so as we are not adding a ConstantInstr that then gest unused.

[regis, 2014-05-07 01:26:26]

On 2014/05/06 23:51:56, srdjan wrote:
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> runtime/vm/flow_graph_builder.cc:1423: ReturnDefinition(new
> ConstantInstr(Bool::Get(!negate_result)));
> On 2014/05/06 23:36:57, regis wrote:
> > Will this other optimization cause the same issue?
> 
> Do not think so as we are not adding a ConstantInstr that then gest unused.

We do add ConstantInstr. I do not see the difference.

Actually, if you replace 'double' by 'Object' in the test, you will get the same
assert fault after applying your change.

I wonder if removing these simple optimizations is the correct fix. There must
be a better way to avoid this crash.

[srdjan, 2014-05-07 16:01:51]

On 2014/05/07 01:26:26, regis wrote:
> On 2014/05/06 23:51:56, srdjan wrote:
> >
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> > File runtime/vm/flow_graph_builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> > runtime/vm/flow_graph_builder.cc:1423: ReturnDefinition(new
> > ConstantInstr(Bool::Get(!negate_result)));
> > On 2014/05/06 23:36:57, regis wrote:
> > > Will this other optimization cause the same issue?
> > 
> > Do not think so as we are not adding a ConstantInstr that then gest unused.
> 
> We do add ConstantInstr. I do not see the difference.
> 
> Actually, if you replace 'double' by 'Object' in the test, you will get the
same
> assert fault after applying your change.
> 
> I wonder if removing these simple optimizations is the correct fix. There must
> be a better way to avoid this crash.

Yes, you are correct, I can reproduce the crash with that as well. Removed both
unnecessary optimizations.

[srdjan, 2014-05-07 17:27:01]

On 2014/05/07 16:01:51, srdjan wrote:
> On 2014/05/07 01:26:26, regis wrote:
> > On 2014/05/06 23:51:56, srdjan wrote:
> > >
> >
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> > > File runtime/vm/flow_graph_builder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/269273002/diff/1/runtime/vm/flow_graph_builde...
> > > runtime/vm/flow_graph_builder.cc:1423: ReturnDefinition(new
> > > ConstantInstr(Bool::Get(!negate_result)));
> > > On 2014/05/06 23:36:57, regis wrote:
> > > > Will this other optimization cause the same issue?
> > > 
> > > Do not think so as we are not adding a ConstantInstr that then gest
unused.
> > 
> > We do add ConstantInstr. I do not see the difference.
> > 
> > Actually, if you replace 'double' by 'Object' in the test, you will get the
> same
> > assert fault after applying your change.
> > 
> > I wonder if removing these simple optimizations is the correct fix. There
must
> > be a better way to avoid this crash.
> 
> Yes, you are correct, I can reproduce the crash with that as well. Removed
both
> unnecessary optimizations.

Implemented Regis' suggestions:
- Restore optimization in flow graph builder that eliminates type tests agains
dynamic (frequent case in checked mode)
- Prevent EffectsGraphVisitor from emitting constant instructions (no effect)

[srdjan, 2014-05-07 17:40:25]

Committed patchset #5 manually as r35855 (presubmit successful).