AOT: Fix bug in receiver type propagation after unique selector calls.

AOT: Fix bug in receiver type propagation after unique selector calls.

Insert Redefinition instructions to prohibit unsafe code motion. If we propagate
the receiver type downwards from a call to a unique selector, we must not hoist
instructions that were optimized using this type information across the call.

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/890bfa80154227e1b5fc0e15d0c0ef5f6b16e013

[Florian Schneider, 2016-11-15 22:53:32]

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

[Vyacheslav Egorov (Google), 2016-11-16 10:16:34]

LGTM but consider the comment.

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:294: // Add redefinition of the receiver to prevent
code motion across
I don't think renaming here is "future proof", though I think it works right
now. 

My main concern is that SSA value might have unrenamed aliases which are later
removed by optimization passes like load forwarding. 

For example:

foofoo(obj) {
  final objAlias = obj;
  closure() => objAlias;
  var res = 0;
  for (var i = 0; i < 2; i++) {
    obj._uniqueSelector();
    res += objAlias.uniqueField;
  }
  return res;
}

Here we will not rename anything and later store-to-load forwarding will replace
objAlias inside the loop with obj. 

Loop body before the LICM looks like this:

B3[target]:28
    PushArgument(v3)
    PolymorphicInstanceCall:6( _uniqueSelector@15186008, v3 IC[1: A cnt:1
trgt:'A._uniqueSelector@15186008'] WITH-CHECKS)
    v27 <- Redefinition(v3 T{A}) T{A}
    v18 <- LoadField(v4, 24, immutable=1)
    PushArgument(v18)
    v20 <- PolymorphicInstanceCall:8( get:uniqueField, v18 IC[1: A cnt:1
trgt:'A.get_uniqueField'] WITH-CHECKS)

So if we happen to run CSE before LICM then things will break again: because v18
will be replaced by v3 not with v27.

I think it's more future-proof to insert redifitions in the type propagator.
Additionally type propagator does traverse the dominator tree so it can rename
uses as it goes.

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:301:
FlowGraph::RenameDominatedUses(call->ArgumentAt(0), redefinition,
There might be no dominated uses. Maybe check that redefinition after this - if
it does not have any uses, remove it from the graph (or better allocate
redefinition lazily - when the first use to rename is found).

[Vyacheslav Egorov (Google), 2016-11-16 12:55:06]

https://codereview.chromium.org/2498073004/diff/20001/tests/language/vm/optim...
File tests/language/vm/optimized_unique_selector_test.dart (right):

https://codereview.chromium.org/2498073004/diff/20001/tests/language/vm/optim...
tests/language/vm/optimized_unique_selector_test.dart:13: var res = 0;
incorrect indentation

[Florian Schneider, 2016-11-16 17:55:18]

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:294: // Add redefinition of the receiver to prevent
code motion across
On 2016/11/16 10:16:34, Vyacheslav Egorov (Google) wrote:
> I don't think renaming here is "future proof", though I think it works right
> now. 
> 
> My main concern is that SSA value might have unrenamed aliases which are later
> removed by optimization passes like load forwarding. 
> 
> For example:
> 
> foofoo(obj) {
>   final objAlias = obj;
>   closure() => objAlias;
>   var res = 0;
>   for (var i = 0; i < 2; i++) {
>     obj._uniqueSelector();
>     res += objAlias.uniqueField;
>   }
>   return res;
> }
> 
> Here we will not rename anything and later store-to-load forwarding will
replace
> objAlias inside the loop with obj. 
> 
> Loop body before the LICM looks like this:
> 
> B3[target]:28
>     PushArgument(v3)
>     PolymorphicInstanceCall:6( _uniqueSelector@15186008, v3 IC[1: A cnt:1
> trgt:'A._uniqueSelector@15186008'] WITH-CHECKS)
>     v27 <- Redefinition(v3 T{A}) T{A}
>     v18 <- LoadField(v4, 24, immutable=1)
>     PushArgument(v18)
>     v20 <- PolymorphicInstanceCall:8( get:uniqueField, v18 IC[1: A cnt:1
> trgt:'A.get_uniqueField'] WITH-CHECKS)
> 
> So if we happen to run CSE before LICM then things will break again: because
v18
> will be replaced by v3 not with v27.
> 
> I think it's more future-proof to insert redifitions in the type propagator.
> Additionally type propagator does traverse the dominator tree so it can rename
> uses as it goes.

Agree. I remember we had this issue already in a different context where we
should insert redefinitions during type propagation.

I'll add this test case for now. We don't propagate types through aliases,
that's what currently prevents get:uniqueField from being inlined.

https://codereview.chromium.org/2498073004/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:301:
FlowGraph::RenameDominatedUses(call->ArgumentAt(0), redefinition,
On 2016/11/16 10:16:34, Vyacheslav Egorov (Google) wrote:
> There might be no dominated uses. Maybe check that redefinition after this -
if
> it does not have any uses, remove it from the graph (or better allocate
> redefinition lazily - when the first use to rename is found). 

Done. For now, I'll remove the redefinition eagerly if there are no uses here.

https://codereview.chromium.org/2498073004/diff/20001/tests/language/vm/optim...
File tests/language/vm/optimized_unique_selector_test.dart (right):

https://codereview.chromium.org/2498073004/diff/20001/tests/language/vm/optim...
tests/language/vm/optimized_unique_selector_test.dart:13: var res = 0;
On 2016/11/16 12:55:06, Vyacheslav Egorov (Google) wrote:
> incorrect indentation 

Done.

[Florian Schneider, 2016-11-16 18:14:45]

Description was changed from

==========
AOT: Fix bug in receiver type propagation after unique selector calls.

Insert Redefinition instructions to prohibit unsafe code motion. If we propagate
the receiver type downwards from a call to a unique selector, we must not hoist
instructions that were optimized using this type information across the call.
==========

to

==========
AOT: Fix bug in receiver type propagation after unique selector calls.

Insert Redefinition instructions to prohibit unsafe code motion. If we propagate
the receiver type downwards from a call to a unique selector, we must not hoist
instructions that were optimized using this type information across the call.

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/890bfa80154227e1b5fc0e15d0c0ef5f6b16e013
==========

[Florian Schneider, 2016-11-16 18:14:46]

Committed patchset #3 (id:40001) manually as
890bfa80154227e1b5fc0e15d0c0ef5f6b16e013 (presubmit successful).