[dart2js] Take more care generating read-modify-write expressions

Bug: #54823
Change-Id: I97ed64c6b487e989fd95704916af0b3aa4cc1e0e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350407
Commit-Queue: Stephen Adams <sra@google.com>
Reviewed-by: Nate Biggs <natebiggs@google.com>

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

@@ -983,6 +983,64 @@ class SsaInstructionMerger extends HBaseVisitor<void> implements CodegenPhase {
     assert(false);
   }
 
+  @override
+  void visitReadModifyWrite(HReadModifyWrite instruction) {
+    if (instruction.isPreOp || instruction.isPostOp) {
+      analyzeInputs(instruction, 0);
+      return;
+    }
+    assert(instruction.isAssignOp);
+    // Generate-at-use is valid for the value operand (t1) if the expression
+    // tree for t1 does not change the order of effects or exceptions with
+    // respect to reading the field of the receiver (t2).
+    //
+    //     t1 = foo();
+    //     t2 = ...
+    //     t2.field += t1;
+    //
+    // 1. If the read of `t2.field` can throw, we can't move `t1` into the
+    //    use-site if some part of the expression tree for `t1` can throw.
+    //
+    // 2. If the expression for `t1` potentially modifies `t2.field`, we can't
+    //    move `t1` past the load `t2.field`.
+    //
+    // TODO(48243): If instruction merging was smarter about effects and was
+    // able to change the order of instructions that read non-aliased fields
+    // this analysis could probably be folded into the normal algorithm by
+    // having HReadModifyWrite have two SideEffects to model the read
+    // indepentently of the write.
+
+    bool throwCheck = instruction.canThrow(_abstractValueDomain);
+
+    bool isSafeSubexpression(HInstruction expression) {
+      // If an expression value is used in more than one place it will be
+      // assigned to a JavaScript variable.
+      if (expression.usedBy.length > 1) return true;
+
+      // Expressions that are generated as JavaScript statements have their
+      // value stored in a variable.
+      if (expression.isJsStatement()) return true;
+
+      // Condition 1.
+      if (throwCheck && expression.canThrow(_abstractValueDomain)) return false;
+
+      // Condition 2.
+      if (expression.sideEffects.changesInstanceProperty()) return false;
+
+      // Many phis end up as JavaScript variables, which would be just fine as
+      // part of the value expression. Since SsaConditionMerger is a separate
+      // pass we can't tell if this phi will become a generate-at-use expression
+      // that is invalid as a subexpression of the value expression.
+      if (expression is HPhi) return false;
+
+      return expression.inputs.every(isSafeSubexpression);
+    }
+
+    if (isSafeSubexpression(instruction.value)) {
+      analyzeInputs(instruction, 0);
+    }
+  }
+
   void tryGenerateAtUseSite(HInstruction instruction) {
     if (instruction.isControlFlow()) return;
     markAsGenerateAtUseSite(instruction);
@@ -1185,6 +1243,12 @@ class SsaConditionMerger extends HGraphVisitor implements CodegenPhase {
     // A [HCheck] instruction with control flow uses its input
     // multiple times, so we avoid generating it at use site.
     if (user is HCheck && user.isControlFlow()) return false;
+
+    // A read-modify-write like `o.field += value` reads the field before
+    // evaluating the value, so if we generate [input] at the value, the order
+    // of field reads may be changed.
+    if (user is HReadModifyWrite && input == user.inputs.last) return false;
+
     // Avoid code motion into a loop.
     return user.hasSameLoopHeaderAs(input);
   }

tests/web/regress/issue/54823_test.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2024, 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.
+
+import 'package:expect/expect.dart';
+
+class V {
+  String buf = "";
+
+  void write(String value) {
+    buf += value;
+  }
+
+  String recurseA(int i) {
+    if (i > 0) {
+      write(recurseA(i - 1));
+      return '[$i]';
+    } else {
+      return '[0]';
+    }
+  }
+
+  String recurseB(int i) {
+    if (i > 0) {
+      write('x' + recurseB(i - 1));
+      return '[$i]';
+    } else {
+      return '[0]';
+    }
+  }
+}
+
+void main() {
+  final v1 = V();
+  v1.recurseA(3);
+  Expect.equals('[0][1][2]', v1.buf);
+
+  final v2 = V();
+  v2.recurseB(3);
+  Expect.equals('x[0]x[1]x[2]', v2.buf);
+}

tests/web/regress/issue/54823b_test.dart

@@ -0,0 +1,23 @@
+// Copyright (c) 2024, 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.
+
+@pragma('dart2js:disable-inlining')
+library;
+
+import 'package:expect/expect.dart';
+
+final StringBuffer sb = StringBuffer();
+
+class A {
+  @override
+  String toString() {
+    sb.write('x');
+    return 'A()';
+  }
+}
+
+void main() {
+  sb.write(A());
+  Expect.equals('xA()', sb.toString());
+}

tests/web/regress/issue/54823c_test.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2024, 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.
+
+import 'package:expect/expect.dart';
+
+class A {
+  String a = 'a';
+
+  @pragma('dart2js:never-inline')
+  String f(String s) {
+    return a = a + s;
+  }
+
+  @pragma('dart2js:never-inline')
+  String foo(String x) {
+    // This expression can be generated as a JavaScript '?:' conditional
+    // expression, but should not be on the right side of `this.a += ...`.
+    final q = a == x ? f('1') : f('2');
+    return a = a + q;
+  }
+
+  @pragma('dart2js:never-inline')
+  String bar(String x) {
+    // This expression can be generated as a JavaScript '?:' conditional
+    // expression, but should not be on the right side of `this.a += ... + 'x'`.
+    final q = a == x ? f('1') : f('2');
+    final r = q + 'x';
+    return a = a + r;
+  }
+}
+
+void main() {
+  Expect.equals('a1a1', A().foo('a'));
+  Expect.equals('a2a2', A().foo('b'));
+  Expect.equals('a1a1x', A().bar('a'));
+  Expect.equals('a2a2x', A().bar('b'));
+}