VM: Fix receiver type propagation in presence of try-catch.

VM: Fix receiver type propagation in presence of try-catch.

With catch blocks appearing as additional function entry blocks,
there can be phis for the receiver (parameter 0).

This CL fixes the problem that the receiver type information
was lost in the presence of try-catch.

BUG=
R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/2783afd6072bd3937f0d3f1d99032f35a09de4be

[Florian Schneider, 2016-03-29 23:41:46]

fschneider@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2016-03-30 16:59:21]

lgtm

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:377: if (def->IsParameter() &&
(def->AsParameter()->index() == 0)) return true;
Maybe

if (def->IsParameter() return (def->AsParameter()->index() == 0);

?

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:383: if (!IsReceiver(phi->InputAt(i)->definition(),
processed)) return false;
Please add a comment that we are not reusing processed between IsReceiver
invocations.

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:402: IsReceiver(callee_receiver,
I find it somewhat expensive not only to repeat this analysis again and again
for phis but also to allocate processed even if it is certainly not needed (e.g.
callee_receiver is not a Phi or Parameter).

PhiInstr has bool is_alive_; - so maybe put a uint8_t is_receiver next to it and
have it UNKNOWN, RECEIVER, NOT_RECEIVER :)

[Florian Schneider, 2016-03-30 21:03:57]

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:377: if (def->IsParameter() &&
(def->AsParameter()->index() == 0)) return true;
On 2016/03/30 16:59:20, Vyacheslav Egorov (Google) wrote:
> Maybe
> 
> if (def->IsParameter() return (def->AsParameter()->index() == 0);
> 
> ? 

Done.

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:383: if (!IsReceiver(phi->InputAt(i)->definition(),
processed)) return false;
On 2016/03/30 16:59:20, Vyacheslav Egorov (Google) wrote:
> Please add a comment that we are not reusing processed between IsReceiver
> invocations.

Done. Rewrote computation to store the result at each phi.

https://codereview.chromium.org/1841073003/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:402: IsReceiver(callee_receiver,
On 2016/03/30 16:59:20, Vyacheslav Egorov (Google) wrote:
> I find it somewhat expensive not only to repeat this analysis again and again
> for phis but also to allocate processed even if it is certainly not needed
(e.g.
> callee_receiver is not a Phi or Parameter).
> 
> PhiInstr has bool is_alive_; - so maybe put a uint8_t is_receiver next to it
and
> have it UNKNOWN, RECEIVER, NOT_RECEIVER :)

Good point. It should not take extra space anyway. I only need to do the
analysis when try-catch is present, too.

[Florian Schneider, 2016-03-30 22:55:15]

Description was changed from

==========
VM: Fix receiver type propagation in presence of try-catch.

With catch blocks appearing as additional function entry blocks,
there can be phis for the receiver (parameter 0).

This CL fixes the problem that the receiver type information
was lost in the presence of try-catch.

BUG=
==========

to

==========
VM: Fix receiver type propagation in presence of try-catch.

With catch blocks appearing as additional function entry blocks,
there can be phis for the receiver (parameter 0).

This CL fixes the problem that the receiver type information
was lost in the presence of try-catch.

BUG=
R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2783afd6072bd3937f0d3f1d99032f35a09de4be
==========

[Florian Schneider, 2016-03-30 22:55:16]

Committed patchset #3 (id:40001) manually as
2783afd6072bd3937f0d3f1d99032f35a09de4be (presubmit successful).

[Vyacheslav Egorov (Google), 2016-03-31 05:00:58]

https://codereview.chromium.org/1841073003/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/1841073003/diff/40001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:388: ComputeIsReceiverRecursive(def->AsPhi(),
processed);
Something is not right here. Imagine situation:

x <- phi (y, z)
y <- phi (x, x)
z <- not-receiver 

in this case we will mark y as a receiver phi as far as I can see. 

I think the right way to do it is to not use bitvector at all, but instead mark
phi optimistically PhiInstr::kReceiver before the loop and then when it proves
to be wrong walk all uses of the phi and mark phis among them as kNotReceiver. 

If possible it would be good to have a test case covering this corner case too.

[Florian Schneider, 2016-03-31 17:13:54]

https://codereview.chromium.org/1841073003/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/1841073003/diff/40001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:388: ComputeIsReceiverRecursive(def->AsPhi(),
processed);
On 2016/03/31 05:00:58, Vyacheslav Egorov (Google) wrote:
> Something is not right here. Imagine situation:
> 
> x <- phi (y, z)
> y <- phi (x, x)
> z <- not-receiver 
> 
> in this case we will mark y as a receiver phi as far as I can see. 
> 
> I think the right way to do it is to not use bitvector at all, but instead
mark
> phi optimistically PhiInstr::kReceiver before the loop and then when it proves
> to be wrong walk all uses of the phi and mark phis among them as kNotReceiver.

> 
> If possible it would be good to have a test case covering this corner case
too. 

You're right. The current way does not the compute the property correctly for
the phis visited. I could also only compute the property for the phi of the
initial call to ComputeRecursive, but I think I'll do a pass over all phis.