When attempting to to inline a field getter we use the incoming cid. That type may differ between v…

When attempting to to inline a field getter we use the incoming cid. That type may differ between value()->Type() and value->definition()->Type() if the definition is a load field with a guarded cid. Therefore, if value's type is dynamic we check definition's type as well.
Committed: https://code.google.com/p/dart/source/detail?r=20548

[Vyacheslav Egorov (Google), 2013-03-26 18:56:13]

LGTM

but consider alternative approach that keeps reaching types attached to Values
in sync with definition.

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

https://codereview.chromium.org/12843043/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:90: if (cid == kDynamicCid) {
How about a bit different approach:

In Definition::ReplaceUsesWith(Definition* other) when iterating over input uses
we can check if current->Type() == this->Type() and if this is the case reset
current->type_ to NULL.

To avoid doing unnecessary updates we can have bool argument to
ReplaceUsesWith/ReplaceWith that controls whether or not we update types.

And in the FlowGraphOptimizer we can have a member that says update_types_ that
is true when we are in the ApplyClassIds.

In this case we will keep types more up to date, and will need to have any
special casing here.

[srdjan, 2013-03-26 20:34:52]

PTAL

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

https://codereview.chromium.org/12843043/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:90: if (cid == kDynamicCid) {
On 2013/03/26 18:56:13, Vyacheslav Egorov (Google) wrote:
> How about a bit different approach:
> 
> In Definition::ReplaceUsesWith(Definition* other) when iterating over input
uses
> we can check if current->Type() == this->Type() and if this is the case reset
> current->type_ to NULL.
> 
> To avoid doing unnecessary updates we can have bool argument to
> ReplaceUsesWith/ReplaceWith that controls whether or not we update types.
> 
> And in the FlowGraphOptimizer we can have a member that says update_types_
that
> is true when we are in the ApplyClassIds.
> 
> In this case we will keep types more up to date, and will need to have any
> special casing here.

Yes, update the type in ReplaceUsesWith. In order to implement simple code, I
decided to always set reaching type to NULL. Did not measure negative impact on
compilation speed of dart2js.

[Vyacheslav Egorov (Google), 2013-03-26 20:46:12]

lgtm

https://codereview.chromium.org/12843043/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12843043/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.cc:531: current->SetReachingType(NULL);
I think guarding it with 

if (current->Type() == this->Type()) current->SetReachingType(NULL);

is better because otherwise you will discard some useful inferred types, e.g. if
result of the call was passing through the CheckClass.

[srdjan, 2013-03-26 21:23:34]

https://codereview.chromium.org/12843043/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12843043/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.cc:531: current->SetReachingType(NULL);
On 2013/03/26 20:46:12, Vyacheslav Egorov (Google) wrote:
> I think guarding it with 
> 
> if (current->Type() == this->Type()) current->SetReachingType(NULL);
> 
> is better because otherwise you will discard some useful inferred types, e.g.
if
> result of the call was passing through the CheckClass.

Done.

[srdjan, 2013-03-26 22:02:14]

Committed patchset #4 manually as r20548 (presubmit successful).