Fixup redefinitions before doing code motion

Fixup redefinitions before doing code motion

Also use redefined receiver consistently when inlining recognized methods.

This CL re-enables type propagation from CheckClassId instructions which
was disabled as a temporary stop-gap to prevent illegal code motion.
(reverts https://codereview.chromium.org/1928633002)

Now uses of redefined values that are dominated by the redefinition are renamed before doing LICM.

Related to #26347.

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/521526d25903e15b002f82a64b09f2018b53fbba

[Florian Schneider, 2017-02-22 22:12:08]

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

[Florian Schneider, 2017-02-22 23:32:57]

Description was changed from

==========
Fixup redefinitions before doing code motion

Also use redefined receiver consistently when inlining recognized methods.

This CL re-enables type propagation from CheckClassId instructions which
was disabled as a temporary stop-gap to prevent illegal code motion.
(reverts https://codereview.chromium.org/1928633002)

Now uses of redefined values that are dominated by the redefinition are renamed
before doing LICM.
==========

to

==========
Fixup redefinitions before doing code motion

Also use redefined receiver consistently when inlining recognized methods.

This CL re-enables type propagation from CheckClassId instructions which
was disabled as a temporary stop-gap to prevent illegal code motion.
(reverts https://codereview.chromium.org/1928633002)

Now uses of redefined values that are dominated by the redefinition are renamed
before doing LICM.

Related to #26347.
==========

[Vyacheslav Egorov (Google), 2017-02-23 13:47:27]

LGTM

Though I would really preferred if we tightly tied redefinitions and type
propagation. Currently we expect that we have a redefinition that would guard
uses dominated by comparisos of class-id - but it feels very fragile.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:897: licm.OptimisticallySpecializeSmiPhis();
I think we don't need to fixup redefinitions here - this pass only hoists
CheckSmi instructions.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:930: if (DominatorBasedCSE::Optimize(flow_graph)) {
We do load forwarding here so we might introduce new uses of redefined values,
e.g. by replacing some LoadFieldInstr(...) with concrete values. 

So we need to have flow_graph->FixupRedefinitions() past this point.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:947: LICM licm(flow_graph);
I think we should fixup redefinitions here.

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

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:491: // Verify that a redefinition domiates all uses of
the redefined value.
s/domiates/dominates/

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:2129: void FlowGraph::FixupRedefinitions() {
FixupRedefinitions(...) is a confusing name - maybe
RedefineUsesDominatedByRedefinitions() or something like that.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:2136: Definition* original =
redefinition->value()->definition();
Can we have multiple Redefinitions(), e.g.

v0 <- something

| v1 <- Redefinition(v0)
|
| | v2 <- Redefinition(v0)
| |
| | use(v1)

in this case we will not rename use(v1) to v2 which might lead to incorrect code
motion.

[Florian Schneider, 2017-02-23 19:26:39]

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:897: licm.OptimisticallySpecializeSmiPhis();
On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> I think we don't need to fixup redefinitions here - this pass only hoists
> CheckSmi instructions.

Done.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:930: if (DominatorBasedCSE::Optimize(flow_graph)) {
On 2017/02/23 13:47:26, Vyacheslav Egorov (Google) wrote:
> We do load forwarding here so we might introduce new uses of redefined values,
> e.g. by replacing some LoadFieldInstr(...) with concrete values. 
> 
> So we need to have flow_graph->FixupRedefinitions() past this point.

Done.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:947: LICM licm(flow_graph);
On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> I think we should fixup redefinitions here.

Done.

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

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:491: // Verify that a redefinition domiates all uses of
the redefined value.
On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> s/domiates/dominates/

Done.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:2129: void FlowGraph::FixupRedefinitions() {
On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> FixupRedefinitions(...) is a confusing name - maybe
> RedefineUsesDominatedByRedefinitions() or something like that.

Done.

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:2136: Definition* original =
redefinition->value()->definition();
On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> Can we have multiple Redefinitions(), e.g.
> 
> v0 <- something
> 
> | v1 <- Redefinition(v0)
> |
> | | v2 <- Redefinition(v0)
> | |
> | | use(v1)
> 
> in this case we will not rename use(v1) to v2 which might lead to incorrect
code
> motion. 
> 

I have not seen this case: I think if redefinitions are properly inserted, v2
would redefine v1 if there is a dependency. Otherwise the two are independent.
How about we do CSE of redefinitions to make sure that it does not occur?

[Florian Schneider, 2017-02-23 20:35:02]

Description was changed from

==========
Fixup redefinitions before doing code motion

Also use redefined receiver consistently when inlining recognized methods.

This CL re-enables type propagation from CheckClassId instructions which
was disabled as a temporary stop-gap to prevent illegal code motion.
(reverts https://codereview.chromium.org/1928633002)

Now uses of redefined values that are dominated by the redefinition are renamed
before doing LICM.

Related to #26347.
==========

to

==========
Fixup redefinitions before doing code motion

Also use redefined receiver consistently when inlining recognized methods.

This CL re-enables type propagation from CheckClassId instructions which
was disabled as a temporary stop-gap to prevent illegal code motion.
(reverts https://codereview.chromium.org/1928633002)

Now uses of redefined values that are dominated by the redefinition are renamed
before doing LICM.

Related to #26347.

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521526d25903e15b002f82a64b09f2018b53fbba
==========

[Florian Schneider, 2017-02-23 20:35:03]

Committed patchset #3 (id:40001) manually as
521526d25903e15b002f82a64b09f2018b53fbba (presubmit successful).

[Vyacheslav Egorov (Google), 2017-02-23 20:53:18]

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

https://codereview.chromium.org/2709093003/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:2136: Definition* original =
redefinition->value()->definition();
On 2017/02/23 19:26:35, Florian Schneider wrote:
> On 2017/02/23 13:47:27, Vyacheslav Egorov (Google) wrote:
> > Can we have multiple Redefinitions(), e.g.
> > 
> > v0 <- something
> > 
> > | v1 <- Redefinition(v0)
> > |
> > | | v2 <- Redefinition(v0)
> > | |
> > | | use(v1)
> > 
> > in this case we will not rename use(v1) to v2 which might lead to incorrect
> code
> > motion. 
> > 
> 
> I have not seen this case: I think if redefinitions are properly inserted, v2
> would redefine v1 if there is a dependency. Otherwise the two are independent.
> How about we do CSE of redefinitions to make sure that it does not occur?

I think this can occur in the situation like this:

class A {
  foo(obj) => obj.x();
}

bar(obj) {
  obj.foo(obj); // inline foo polymorphically here and then inline obj.x
polymorphically
}

but then our type propagation will take care of it.

I did not try to craft example where it goes south, but I will try.