[dart2js] Fix variable allocator live-range bug

The controlling condition of a control-flow operator (?:, &&, ||)
needs to have the same live range as the other operands.

Change-Id: Iee999a1b400ef35ccbb761fcd7266952c9a95be9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192605
Reviewed-by: Mayank Patke <[email protected]>
Commit-Queue: Stephen Adams <[email protected]>

Author: rakudrama

pkg/compiler/lib/src/ssa/variable_allocator.dart

@@ -168,14 +168,22 @@ class SsaLiveIntervalBuilder extends HBaseVisitor with CodegenPhase {
   int instructionId = 0;
 
   /// The liveIns of basic blocks.
-  final Map<HBasicBlock, LiveEnvironment> liveInstructions;
+  final Map<HBasicBlock, LiveEnvironment> liveInstructions = {};
 
   /// The live intervals of instructions.
-  final Map<HInstruction, LiveInterval> liveIntervals;
+  final Map<HInstruction, LiveInterval> liveIntervals = {};
+
+  /// Controlling conditions for control-flow operators. Control-flow operators,
+  /// e.g. `c ? a : b`, have a condition input `c` from the HIf node
+  /// at the top of the control flow diamond as well as the HPhi inputs for `a`
+  /// and `b` at the bottom of the diamond.
+  final Map<HInstruction, HInstruction> _phiToCondition = {};
 
-  SsaLiveIntervalBuilder(this.generateAtUseSite, this.controlFlowOperators)
-      : liveInstructions = new Map<HBasicBlock, LiveEnvironment>(),
-        liveIntervals = new Map<HInstruction, LiveInterval>();
+  SsaLiveIntervalBuilder(this.generateAtUseSite, this.controlFlowOperators) {
+    for (HIf ifNode in controlFlowOperators) {
+      _phiToCondition[ifNode.joinBlock.phis.first] = ifNode.condition;
+    }
+  }
 
   @override
   void visitGraph(HGraph graph) {
@@ -187,6 +195,12 @@ class SsaLiveIntervalBuilder extends HBaseVisitor with CodegenPhase {
 
   void markInputsAsLiveInEnvironment(
       HInstruction instruction, LiveEnvironment environment) {
+    if (instruction is HPhi) {
+      HInstruction condition = _phiToCondition[instruction];
+      if (condition != null) {
+        markAsLiveInEnvironment(condition, environment);
+      }
+    }
     for (int i = 0, len = instruction.inputs.length; i < len; i++) {
       markAsLiveInEnvironment(instruction.inputs[i], environment);
     }
@@ -257,21 +271,7 @@ class SsaLiveIntervalBuilder extends HBaseVisitor with CodegenPhase {
 
   @override
   void visitBasicBlock(HBasicBlock block) {
-    LiveEnvironment environment =
-        new LiveEnvironment(liveIntervals, instructionId);
-
-    // If the control flow instruction in this block will actually be
-    // inlined in the codegen in the join block, we need to make
-    // whatever is used by that control flow instruction as live in
-    // the join block.
-    if (controlFlowOperators.contains(block.last)) {
-      HIf ifInstruction = block.last;
-      HBasicBlock joinBlock = ifInstruction.joinBlock;
-      if (generateAtUseSite.contains(joinBlock.phis.first)) {
-        markInputsAsLiveInEnvironment(
-            ifInstruction, liveInstructions[joinBlock]);
-      }
-    }
+    LiveEnvironment environment = LiveEnvironment(liveIntervals, instructionId);
 
     // Add to the environment the liveIn of its successor, as well as
     // the inputs of the phis of the successor that flow from this block.

tests/web_2/regress/183227419_test.dart

@@ -0,0 +1,56 @@
+// Copyright (c) 2021, 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.
+
+// @dart=2.10
+
+// Regression test for variable allocator live range bug.
+//
+// Pre-fix, this program would crash because t2 was reused before it was dead.
+//
+//     algo$1: function(n) {
+//       var t1 = this.ax,
+//         t2 = t1 == null;
+//       if ((t2 ? null : t1.a) == null)
+//         return;
+//       t2 = n + 1;  // the old value of t2 is used below!
+//       (t2 ? null : t1.a).parameters.addAll$2(0, t2, t2);
+//     }
+//
+// Several things were necessary to tickle the bug:
+//
+//  [A] The repeated test that gets shared from the two null-aware `ax?.a`
+//      expressions.
+//  [B] A null-aware `parameters?.` that is optimized away because `parameters`
+//      is always non-null.
+//  [C] A repeated expression after the optimized null-aware access.
+
+class Thing {
+  @pragma('dart2js:noInline')
+  void addAll(x, y) {}
+}
+
+class A {
+  Thing parameters = Thing();
+}
+
+class AX {
+  A a;
+  AX(this.a);
+}
+
+class Host {
+  AX ax;
+  Host(this.ax);
+
+  algo(int n) {
+    if (ax?.a == null) return;
+    ax?.a.parameters?.addAll(n + 1, n + 1);
+  }
+}
+
+main() {
+  Host(null).algo(1);
+  Host(AX(A())).algo(2);
+  Host(AX(null)).algo(3);
+}