[vm/compiler] Check def/use relation for PushArguments in environment.

Fix an AOT call specialization that broke this relationship.

When printing environments in the FlowGraphPrinter, also print the
underlying value for any PushArguments.

Change-Id: I39803b7d995abac720702ea4b9d6a78fcbc45d4c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/115981
Commit-Queue: Teagan Strickland <sstrickl@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

runtime/tests/vm/dart/getter_closure_call_generic_test.dart

@@ -0,0 +1,70 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// This regression test targets AotCallSpecializer::TryExpandCallThroughGetter,
+// which previously would create bad references in environments to either
+// push arguments that came later than the environment's instruction or, worse,
+// references to push arguments whose values had not already been calculated at
+// the environment's instruction.
+
+final states = <int>[];
+main() {
+  final x = kTrue ? Sub1() : Sub2();
+  barbar(kTrue, x);
+  print('states = $states');
+}
+
+@pragma('vm:never-inline')
+void barbar(bool kTrue, Base base) {
+  var a = foo();
+  var b = foo2();
+  var c = foo3();
+  var d = foo4();
+
+  try {
+    // rotate
+    final t = a;
+    if (kTrue) a = b;
+    if (kTrue) b = c;
+    if (kTrue) c = d;
+    if (kTrue) d = t;
+
+    base.barGetter(
+        a = addState(c), b = addState(d), c = addState(a), d = addState(b));
+  } catch (e) {
+    print('got $e');
+    print('$a $b $c $d');
+  }
+}
+
+final bool kTrue = int.parse('1') == 1;
+
+@pragma('vm:never-inline')
+int foo() => kTrue ? 1 : 2;
+
+@pragma('vm:never-inline')
+int foo2() => kTrue ? 2 : 3;
+
+@pragma('vm:never-inline')
+int foo3() => kTrue ? 3 : 4;
+
+@pragma('vm:never-inline')
+int foo4() => kTrue ? 4 : 5;
+
+@pragma('vm:never-inline')
+int addState(int i) {
+  states.add(i);
+  return i;
+}
+
+@pragma('vm:never-inline')
+void doit<A>(A x, A x2, A x3, A x4) => throw 'a';
+
+class Base {
+  void Function<A>(A, A, A, A) get barGetter => doit;
+}
+
+class Sub1 extends Base {}
+
+class Sub2 extends Base {}

runtime/tests/vm/dart/getter_closure_call_nongeneric_test.dart

@@ -0,0 +1,51 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// This regression test targets AotCallSpecializer::TryExpandCallThroughGetter,
+// which previously would create bad references in environments to either
+// push arguments that came later than the environment's instruction or, worse,
+// references to push arguments whose values had not already been calculated at
+// the environment's instruction.
+
+import 'getter_closure_call_generic_test.dart' as test;
+
+main() {
+  final x = test.kTrue ? Sub1() : Sub2();
+  barbar(test.kTrue, x);
+  print('states = ${test.states}');
+}
+
+@pragma('vm:never-inline')
+barbar(bool kTrue, Base base) {
+  var a = test.foo();
+  var b = test.foo2();
+  var c = test.foo3();
+  var d = test.foo4();
+  try {
+    // rotate
+    final t = a;
+    if (kTrue) a = b;
+    if (kTrue) b = c;
+    if (kTrue) c = d;
+    if (kTrue) d = t;
+
+    if (!kTrue) b = test.foo();
+
+    base.barGetter(a = test.addState(1), b = test.addState(2),
+        c = test.addState(3), d = test.addState(4));
+  } catch (e) {
+    print('got $e');
+    print('$a $b $c $d');
+  }
+}
+
+class Base {
+  FT get barGetter => test.doit;
+}
+
+class Sub1 extends Base {}
+
+class Sub2 extends Base {}
+
+typedef dynamic FT(dynamic a, dynamic b, dynamic c, dynamic d);

runtime/vm/compiler/aot/aot_call_specializer.cc

@@ -1058,6 +1058,37 @@ void AotCallSpecializer::VisitStaticCall(StaticCallInstr* instr) {
   CallSpecializer::VisitStaticCall(instr);
 }
 
+// Replaces any PushArguments for [call] appearing in [env] with the definition
+// of the PushArgument's value. If [idx] is given, then only PushArguments at
+// that index or later are replaced. If [env] is nullptr or [idx] is greater
+// than the number of arguments to [call], then this function is a no-op.
+static void ReplacePushesInEnvWithValueDefinition(Environment* env,
+                                                  InstanceCallInstr* call,
+                                                  intptr_t idx = 0) {
+  ASSERT(call != nullptr);
+  if (env == nullptr) return;
+  if (idx >= call->ArgumentCount()) return;
+  auto const first_push = call->PushArgumentAt(idx);
+  Environment::DeepIterator it(env);
+  // First, look in the iterator for [first_push].
+  for (; !it.Done(); it.Advance()) {
+    if (it.CurrentValue()->definition() == first_push) break;
+  }
+  if (it.Done()) return;
+  // If we found [first_push], then any other references to later push
+  // arguments should come immediately in the environment, so iterate
+  // and replace as described above. If we hit the end, a non-push argument or
+  // a push argument unrelated to the original call, then stop.
+  for (intptr_t i = idx; i < call->ArgumentCount(); i++) {
+    if (it.Done()) return;
+    auto const value = it.CurrentValue();
+    auto const push = value->definition()->AsPushArgument();
+    if (push != call->PushArgumentAt(i)) break;
+    value->BindToEnvironment(push->value()->definition());
+    it.Advance();
+  }
+}
+
 bool AotCallSpecializer::TryExpandCallThroughGetter(const Class& receiver_class,
                                                     InstanceCallInstr* call) {
   // If it's an accessor call it can't be a call through getter.
@@ -1126,26 +1157,76 @@ bool AotCallSpecializer::TryExpandCallThroughGetter(const Class& receiver_class,
       /*checked_argument_count=*/1,
       thread()->compiler_state().GetNextDeoptId());
 
-  // Insert all new instructions, except .call() invocation into the
-  // graph.
-  for (intptr_t i = 0; i < invoke_get->ArgumentCount(); i++) {
-    InsertBefore(call, invoke_get->PushArgumentAt(i), NULL, FlowGraph::kEffect);
-  }
+  // For any instructions with environments between the old receiver push and
+  // the original call, replace any appearances of the post-receiver
+  // PushArguments from the call in the environment with the underlying value.
+  // This will allow us to put the new PushArguments in different places than
+  // the old ones.
+  for (auto inst = call->PushArgumentAt(receiver_idx)->next(); inst != call;
+       inst = inst->next()) {
+    ASSERT(inst != nullptr);
+    ReplacePushesInEnvWithValueDefinition(inst->env(), call, receiver_idx + 1);
+  }
+
+  // Insert all the new instructions into the graph.
+
+  // First, insert the [invoke_get] instruction, then directly replace the old
+  // TypeArguments push (if any) as well as the old receiver push with the
+  // receiver push for [invoke_get].
+  //
+  // We must insert [invoke_get] right before the old call because local
+  // variables may have been redefined during the evaluation of the arguments to
+  // the old call. If they were, then there may be references to the results of
+  // those evaluations in the environment.
   InsertBefore(call, invoke_get, call->env(), FlowGraph::kValue);
-  for (intptr_t i = 0; i < invoke_call->ArgumentCount(); i++) {
-    InsertBefore(call, invoke_call->PushArgumentAt(i), NULL,
-                 FlowGraph::kEffect);
+  ReplacePushesInEnvWithValueDefinition(invoke_get->env(), call,
+                                        receiver_idx + 1);
+  if (receiver_idx > 0) {
+    call->PushArgumentAt(0)->ReplaceWith(invoke_call->PushArgumentAt(0),
+                                         /*iterator=*/nullptr);
+  }
+  call->PushArgumentAt(receiver_idx)
+      ->ReplaceWith(invoke_get->PushArgumentAt(0), /*iterator=*/nullptr);
+
+  // For the instructions that will replace the original call, we'll also need
+  // to alter their environments. For [invoke_get], we just need to change
+  // pushes that we'll be replacing later, and we do that now after copying
+  // the original call's environment as part of the InsertBefore.
+  ReplacePushesInEnvWithValueDefinition(invoke_get->env(), call,
+                                        receiver_idx + 1);
+
+  // Next, replace the original call with the new .call(...) invocation.
+  // To update the environment for [invoke_call], just change reference to the
+  // receiver PushArgument. All other push arguments will be changed in the
+  // last step.
+  call->ReplaceWith(invoke_call, current_iterator());
+  for (Environment::DeepIterator it(invoke_call->env()); !it.Done();
+       it.Advance()) {
+    auto const val = it.CurrentValue();
+    if (auto const push = val->definition()->AsPushArgument()) {
+      if (push == call->PushArgumentAt(receiver_idx)) {
+        val->BindToEnvironment(invoke_call->PushArgumentAt(receiver_idx));
+        break;  // The push argument will only appear once in the environment.
+      }
+    }
   }
-  // Replace original PushArguments in the graph (mainly env uses).
+
+  // Finally, add all the other push arguments for [invoke_call] between
+  // [invoke_get] and [invoke_call]. For the non-receiver pushes, redirect uses
+  // of the old instruction to the new one (notably, in the environment for
+  // [invoke_call] copied from the original) and remove the old instruction.
+  // (The only other use for [invoke_call]'s receiver push, if any, is in its
+  // environment, which we already handled earlier.)
   ASSERT(call->ArgumentCount() == invoke_call->ArgumentCount());
-  for (intptr_t i = 0; i < call->ArgumentCount(); i++) {
+  InsertBefore(invoke_call, invoke_call->PushArgumentAt(receiver_idx),
+               /*env=*/nullptr, FlowGraph::kEffect);
+  for (intptr_t i = receiver_idx + 1; i < call->ArgumentCount(); i++) {
+    InsertBefore(invoke_call, invoke_call->PushArgumentAt(i), /*env=*/nullptr,
+                 FlowGraph::kEffect);
     call->PushArgumentAt(i)->ReplaceUsesWith(invoke_call->PushArgumentAt(i));
     call->PushArgumentAt(i)->RemoveFromGraph();
   }
 
-  // Replace original call with .call(...) invocation.
-  call->ReplaceWith(invoke_call, current_iterator());
-
   // AOT compiler expects all calls to have an ICData.
   EnsureICData(Z, flow_graph()->function(), invoke_get);
   EnsureICData(Z, flow_graph()->function(), invoke_call);

runtime/vm/compiler/backend/flow_graph_checker.cc

@@ -307,6 +307,8 @@ void FlowGraphChecker::VisitUseDef(Instruction* instruction,
     ASSERT(DefDominatesUse(def, instruction));
     ASSERT(IsInUseList(is_env ? def->env_use_list() : def->input_use_list(),
                        instruction));
+  } else if (def->IsPushArgument()) {
+    ASSERT(DefDominatesUse(def, instruction));
   }
 }
 

runtime/vm/compiler/backend/il.cc

@@ -1302,8 +1302,7 @@ void Value::RemoveFromUseList() {
   } else if (this == def->env_use_list()) {
     def->set_env_use_list(next);
     if (next != NULL) next->set_previous_use(NULL);
-  } else {
-    Value* prev = previous_use();
+  } else if (Value* prev = previous_use()) {
     prev->set_next_use(next);
     if (next != NULL) next->set_previous_use(prev);
   }

runtime/vm/compiler/backend/il_printer.cc

@@ -1147,13 +1147,16 @@ void ParallelMoveInstr::PrintTo(BufferFormatter* f) const {
 
 void Environment::PrintTo(BufferFormatter* f) const {
   f->Print(" env={ ");
-  int arg_count = 0;
+  intptr_t arg_count = 0;
   for (intptr_t i = 0; i < values_.length(); ++i) {
+    auto const value = values_[i];
     if (i > 0) f->Print(", ");
-    if (values_[i]->definition()->IsPushArgument()) {
-      f->Print("a%d", arg_count++);
+    if (auto const push = value->definition()->AsPushArgument()) {
+      f->Print("a%" Pd " (", arg_count++);
+      push->value()->PrintTo(f);
+      f->Print(")");
     } else {
-      values_[i]->PrintTo(f);
+      value->PrintTo(f);
     }
     if ((locations_ != NULL) && !locations_[i].IsInvalid()) {
       f->Print(" [");