Propagate class ids using existing type propagation framework.

Propagate class ids using existing type propagation framework.
Committed: https://code.google.com/p/dart/source/detail?r=10849

[regis, 2012-08-15 22:46:54]

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

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:695: if (phi->propagated_cid() ==
kDynamicCid) {
So far, I have tried not to depend on the type of the result of the previous
iteration, but only on the types of the inputs in the current iteration.
In your case, it is not clear to me if a propagated cid could be dynamic in the
first iteration and change in a following one. I think it should be possible.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:698: // Merge class ids: if any two inputs
have different class ids than result
s/than/then/

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:703: changed = phi->SetPropagatedCid(cid);
This does not look right. You are not merging, but setting the cid of the last
input as the phi's cid.
OK, I understand now when looking at SetPropagatedCid(). See my comment there.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:602: if (definition()->IsBind()) {
Why do you test for a Bind?
You should just forward to the definition, which may be a Phi. See
CompileType().

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.h:159: virtual intptr_t ResultCid() const {
return kDynamicCid; }
This is correct to provide a default implementation. I preferred not to provide
one to make sure I do not forget to handle a computation, especially when we add
a new one later. You could add a TODO to make this abstract later.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.h:2417: bool SetPropagatedCid(intptr_t cid) {
I see now that you are not just setting the propagated cid, but implementing the
merge logic here. SetPropagatedType does not implement the merging logic, but
just sets the type. Note that the merging logic can be different for Phi and
Bind. I think it would be better to move the logic out of here.

[srdjan, 2012-08-16 00:34:46]

All points addressed. PTAL

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

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:695: if (phi->propagated_cid() ==
kDynamicCid) {
On 2012/08/15 22:46:54, regis wrote:
> So far, I have tried not to depend on the type of the result of the previous
> iteration, but only on the types of the inputs in the current iteration.
> In your case, it is not clear to me if a propagated cid could be dynamic in
the
> first iteration and change in a following one. I think it should be possible.

Done.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:698: // Merge class ids: if any two inputs
have different class ids than result
On 2012/08/15 22:46:54, regis wrote:
> s/than/then/

Done.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/flow_gra...
runtime/vm/flow_graph_optimizer.cc:703: changed = phi->SetPropagatedCid(cid);
On 2012/08/15 22:46:54, regis wrote:
> This does not look right. You are not merging, but setting the cid of the last
> input as the phi's cid.
> OK, I understand now when looking at SetPropagatedCid(). See my comment there.

Done.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:602: if (definition()->IsBind()) {
On 2012/08/15 22:46:54, regis wrote:
> Why do you test for a Bind?
> You should just forward to the definition, which may be a Phi. See
> CompileType().

Forwarding to function GetPropagatedCid() that may set the propagated_cid of the
BindInstr.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.h:159: virtual intptr_t ResultCid() const {
return kDynamicCid; }
On 2012/08/15 22:46:54, regis wrote:
> This is correct to provide a default implementation. I preferred not to
provide
> one to make sure I do not forget to handle a computation, especially when we
add
> a new one later. You could add a TODO to make this abstract later.

Added TODO.

https://chromiumcodereview.appspot.com/10830339/diff/6007/runtime/vm/intermed...
runtime/vm/intermediate_language.h:2417: bool SetPropagatedCid(intptr_t cid) {
On 2012/08/15 22:46:54, regis wrote:
> I see now that you are not just setting the propagated cid, but implementing
the
> merge logic here. SetPropagatedType does not implement the merging logic, but
> just sets the type. Note that the merging logic can be different for Phi and
> Bind. I think it would be better to move the logic out of here.

Moved the merge logic out.

[srdjan, 2012-08-16 01:04:10]

Added missing header file and corrected propagated cid of ParameterInstr.

[regis, 2012-08-16 01:07:44]

It looks like you are still working on this change.
Here are some comments.
I'll look at it again tomorrow.

-- Regis

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

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:666: if (!bind->has_propagated_cid())
bind->SetPropagatedCid(kDynamicCid);
I do not understand why you have to do this.
Is the bind outside of the instruction stream?

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:720: if (merged_cid == kIllegalCid) {
I do not think this can happen. This would mean that all inputs are on back
edges.

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/interme...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/interme...
runtime/vm/intermediate_language.h:2420: // - if cid-s differ set to
kDynamicCid.
The code is correct, but the comment is not.

[srdjan, 2012-08-16 01:21:28]

PTAL

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

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:666: if (!bind->has_propagated_cid())
bind->SetPropagatedCid(kDynamicCid);
On 2012/08/16 01:07:44, regis wrote:
> I do not understand why you have to do this.
> Is the bind outside of the instruction stream?

s/bind/push/
PushArgumentInstr is in the instructions stream, but we do not do anything with
it yet. We may want to in the future.

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:720: if (merged_cid == kIllegalCid) {
On 2012/08/16 01:07:44, regis wrote:
> I do not think this can happen. This would mean that all inputs are on back
> edges.

I think it is theoretically possible to order the graph in such a way that all
back edges are after the phi.

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/interme...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10830339/diff/13002/runtime/vm/interme...
runtime/vm/intermediate_language.h:2420: // - if cid-s differ set to
kDynamicCid.
On 2012/08/16 01:07:44, regis wrote:
> The code is correct, but the comment is not. 

Done.

[srdjan, 2012-08-16 01:32:31]

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

https://chromiumcodereview.appspot.com/10830339/diff/13005/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:750:
param->SetPropagatedCid(Class::Handle(param_type.type_class()).id());
This is incorrect (inexact information), removing it. For receiver-type
optimizations we need to use CompileType but we need to know that it cannot be
null.

[regis, 2012-08-16 16:02:47]

LGTM

https://chromiumcodereview.appspot.com/10830339/diff/2009/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10830339/diff/2009/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:325: ASSERT(cid != 1);
What does 1 mean?

[srdjan, 2012-08-16 18:00:56]

https://chromiumcodereview.appspot.com/10830339/diff/2009/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10830339/diff/2009/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:325: ASSERT(cid != 1);
On 2012/08/16 16:02:47, regis wrote:
> What does 1 mean?

Debugging remainder. Thanks for catching it