Cleanup handling of NullType in type propagation.

Cleanup handling of NullType in type propagation.
Remove AssertAssignable type checks from graph (when possible), rather than just
marking them as eliminated.
Committed: https://code.google.com/p/dart/source/detail?r=10782

[regis, 2012-08-15 00:40:39]

Instruction removal is not working yet.

Thanks,
Regis

[srdjan, 2012-08-15 02:07:31]

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:615: #endif
Removed dead code  before submitting

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:673: /* eliminated = */ true);
You cannot eliminate instance of using CompileType, and you are not calling
comp->eliminate() anyway. Remove it?

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:714: // The current bind may have been
removed from the graph.
Can this happen, since you are not removing instructions?

[Kevin Millikin (Google), 2012-08-15 09:04:24]

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:601: // Remove the definition of the input as
well, since it has no uses.
This is not really safe if it happens, because it could have side effects. 
E.g.:

t0 <- Call(....)
AssertAssignable(t0)
...

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:605: // TODO(regis): The above instruction
removal results in an assert fault:
We should just skip this optimization if we're using the virtual frame backend. 
It is going away soon.

[regis, 2012-08-15 17:25:27]

Thanks! Please, have another look.
Instruction removal is now working.

-- Regis

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:601: // Remove the definition of the input as
well, since it has no uses.
On 2012/08/15 09:04:25, kmillikin wrote:
> This is not really safe if it happens, because it could have side effects. 
> E.g.:
> 
> t0 <- Call(....)
> AssertAssignable(t0)
> ...

Thanks! I removed the code removing the unused input. I was trying to understand
the assert fault I was getting.

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:605: // TODO(regis): The above instruction
removal results in an assert fault:
On 2012/08/15 09:04:25, kmillikin wrote:
> We should just skip this optimization if we're using the virtual frame
backend. 
> It is going away soon.

Done. That takes care of the assert fault.

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:615: #endif
On 2012/08/15 02:07:31, srdjan wrote:
> Removed dead code  before submitting

The code is now enabled and working.

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:673: /* eliminated = */ true);
On 2012/08/15 02:07:31, srdjan wrote:
> You cannot eliminate instance of using CompileType, and you are not calling
> comp->eliminate() anyway. Remove it?

I will be able to remove a non-constant value once I know if the value can be
null or not. The code is in place now.

https://chromiumcodereview.appspot.com/10828319/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:714: // The current bind may have been
removed from the graph.
On 2012/08/15 02:07:31, srdjan wrote:
> Can this happen, since you are not removing instructions?

I am now.

[srdjan, 2012-08-15 17:49:56]

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:604: // TODO(regis): Handle constant input
value (not a definition).
s/not a definition/cannot access definition/

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:615: result->ssa_temp_index());
You are not really replacing, but removing. Please adapt output.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:645: // TODO(regis): Remove is_eliminated_
field and support.
I do not think this is correct. Let's discuss offline before submitting.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:690:
comp->value()->CompileTypeIsMoreSpecificThan(comp->type())) {
ditto

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.h (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.h:103: bool is_ssa_;  // TODO(regis): Remove
once virtual frame backend is removed.
const

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
runtime/vm/intermediate_language.cc:169: }
How about making it virtual and implementing in UseVal as return false and in
ConstantVal as return value.IsNull() ?

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
runtime/vm/intermediate_language.cc:1083: // In this case, the optimized strict
equal returns null.
See also TODO(srdjan) above

[srdjan, 2012-08-15 19:55:56]

LGTM

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:645: // TODO(regis): Remove is_eliminated_
field and support.
On 2012/08/15 17:49:56, srdjan wrote:
> I do not think this is correct. Let's discuss offline before submitting.

My mistake. You are doing it only for constant input values. Please verify that
this is ever hit. We may refer to a constant often via a UseVal->
BindInstr->ConstantVal

[regis, 2012-08-15 22:49:17]

Next iteration.

Thanks,
Regis

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:604: // TODO(regis): Handle constant input
value (not a definition).
On 2012/08/15 17:49:56, srdjan wrote:
> s/not a definition/cannot access definition/

Done.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:615: result->ssa_temp_index());
On 2012/08/15 17:49:56, srdjan wrote:
> You are not really replacing, but removing. Please adapt output.

I have removed the instruction, but replaced its uses. I think the text is
correct.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:645: // TODO(regis): Remove is_eliminated_
field and support.
On 2012/08/15 19:55:56, srdjan wrote:
> On 2012/08/15 17:49:56, srdjan wrote:
> > I do not think this is correct. Let's discuss offline before submitting.
> 
> My mistake. You are doing it only for constant input values. Please verify
that
> this is ever hit. We may refer to a constant often via a UseVal->
> BindInstr->ConstantVal

Yes, we only do this for constants, but this will change.
You are correct that constants are never used directly but via a UseVal and
Bind. I have therefore removed the check and replaced  it with an assert here
and in other similar optimizations.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.h (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.h:103: bool is_ssa_;  // TODO(regis): Remove
once virtual frame backend is removed.
On 2012/08/15 17:49:56, srdjan wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
runtime/vm/intermediate_language.cc:169: }
On 2012/08/15 17:49:56, srdjan wrote:
> How about making it virtual and implementing in UseVal as return false and in
> ConstantVal as return value.IsNull() ?

Good point. It is a bit more complex than just returning false from a UseVal,
though, because a UseVal may point to a BindInstr with a ConstantVal as
computation. See my code.

https://chromiumcodereview.appspot.com/10828319/diff/10001/runtime/vm/interme...
runtime/vm/intermediate_language.cc:1083: // In this case, the optimized strict
equal returns null.
On 2012/08/15 17:49:56, srdjan wrote:
> See also TODO(srdjan) above

I removed my TODO, now that the problem is understood.

[srdjan, 2012-08-15 23:41:24]

LGTM

https://chromiumcodereview.appspot.com/10828319/diff/4003/runtime/vm/flow_gra...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/4003/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:751: if
(current_iterator()->Current()->AsBind() == bind) {
No need for AsBind():
current_iterator()->Current() == bind

[regis, 2012-08-15 23:46:06]

Thanks!

https://chromiumcodereview.appspot.com/10828319/diff/4003/runtime/vm/flow_gra...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10828319/diff/4003/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:751: if
(current_iterator()->Current()->AsBind() == bind) {
On 2012/08/15 23:41:25, srdjan wrote:
> No need for AsBind():
> current_iterator()->Current() == bind

Done here and at the other locations.