From 4df361c3c3dcbf4beac87f6f84ce4b9331759ed1 Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Thu, 15 Feb 2024 01:02:39 +0000 Subject: [PATCH] Reapply "[dart2js] Replace phi with controlling condition" This reverts commit aec3ec0244c28a1f637f965e9a72587c068e3527. - Corrected 'controlling condition' detection. - Added test that is incorrectly compiled to infinite loop with incorrect controlling condition detection. See https://github.com/dart-lang/sdk/issues/54115#issuecomment-1944285230 for an image of part of the CFG for `doWhileLoop` where the condition in B4 was previously mis-identified as controlling `phi(true,false)` at B12. Issue: #54115 Change-Id: I0d2c2ff83b202071f6d7050d34de8ff25d05cb22 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352443 Reviewed-by: Mayank Patke Commit-Queue: Stephen Adams --- pkg/compiler/lib/src/ssa/optimize.dart | 71 +++++++++++++++++-- .../test/codegen/data/logical_operators.dart | 34 ++++----- .../codegen/data/phi_to_condition_test.dart | 29 ++++++++ tests/web/regress/issue/54115_loop_test.dart | 29 ++++++++ 4 files changed, 139 insertions(+), 24 deletions(-) create mode 100644 pkg/compiler/test/codegen/data/phi_to_condition_test.dart create mode 100644 tests/web/regress/issue/54115_loop_test.dart diff --git a/pkg/compiler/lib/src/ssa/optimize.dart b/pkg/compiler/lib/src/ssa/optimize.dart index c3e74dae19b..fdbd725ffe6 100644 --- a/pkg/compiler/lib/src/ssa/optimize.dart +++ b/pkg/compiler/lib/src/ssa/optimize.dart @@ -333,13 +333,69 @@ class SsaInstructionSimplifier extends HBaseVisitor } // Simplify some CFG diamonds to equivalent expressions. - simplifyPhis(HBasicBlock block) { - // Is [block] the join point for a simple diamond that generates a single - // phi node? - if (block.phis.isEmpty) return; - HPhi phi = block.phis.first as HPhi; - if (phi.next != null) return; + void simplifyPhis(HBasicBlock block) { if (block.predecessors.length != 2) return; + + // Do 'statement' simplifications first, as they might reduce the number of + // phis to one, enabling an 'expression' simplification. + HInstruction? phi = block.phis.first; + while (phi != null) { + final next = phi.next; + simplifyStatementPhi(block, phi as HPhi); + phi = next; + } + + phi = block.phis.first; + if (phi != null && phi.next == null) { + simplifyExpressionPhi(block, phi as HPhi); + } + } + + /// Simplify a single phi when there are possibly other phis (i.e. the result + /// might not be an expression). + void simplifyStatementPhi(HBasicBlock block, HPhi phi) { + HBasicBlock dominator = block.dominator!; + + // Extract the controlling condition. + final controlFlow = dominator.last; + if (controlFlow is! HIf) return; + HInstruction condition = controlFlow.inputs.single; + + if (condition.isBoolean(_abstractValueDomain).isPotentiallyFalse) return; + + // For the condition to be 'controlling', there must be no way to reach the + // 'else' join from the 'then' branch and vice versa. + if (!dominator.successors[0].dominates(block.predecessors[0])) return; + if (!dominator.successors[1].dominates(block.predecessors[1])) return; + + // condition ? true : false --> condition + // condition ? condition : false --> condition + // condition ? true : condition --> condition + final left = phi.inputs[0]; + final right = phi.inputs[1]; + if ((_isBoolConstant(left, true) || left == condition) && + (_isBoolConstant(right, false) || right == condition)) { + block.rewrite(phi, condition); + block.removePhi(phi); + condition.sourceElement ??= phi.sourceElement; + return; + } + + // condition ? false : true --> !condition + if (_isBoolConstant(left, false) && _isBoolConstant(right, true)) { + HInstruction replacement = HNot(condition, _abstractValueDomain.boolType) + ..sourceElement = phi.sourceElement + ..sourceInformation = phi.sourceInformation; + block.addAtEntry(replacement); + block.rewrite(phi, replacement); + block.removePhi(phi); + return; + } + } + + /// Simplify some CFG diamonds to equivalent expressions. + void simplifyExpressionPhi(HBasicBlock block, HPhi phi) { + // Is [block] the join point for a simple diamond? assert(phi.inputs.length == 2); HBasicBlock b1 = block.predecessors[0]; HBasicBlock b2 = block.predecessors[1]; @@ -350,6 +406,7 @@ class SsaInstructionSimplifier extends HBaseVisitor final controlFlow = dominator.last; if (controlFlow is! HIf) return; HInstruction test = controlFlow.inputs.single; + if (test.usedBy.length > 1) return; bool negated = false; @@ -396,7 +453,7 @@ class SsaInstructionSimplifier extends HBaseVisitor block.removePhi(phi); return; } - // If 'x'is nullable boolean, + // If 'x' is nullable boolean, // // x == null ? true : x ---> !(x == false) // diff --git a/pkg/compiler/test/codegen/data/logical_operators.dart b/pkg/compiler/test/codegen/data/logical_operators.dart index 13f9ff75e4c..aef19254925 100644 --- a/pkg/compiler/test/codegen/data/logical_operators.dart +++ b/pkg/compiler/test/codegen/data/logical_operators.dart @@ -59,6 +59,20 @@ int noopOr(int a) { return (a == 1 || a == 3 || a == 5 || a == 7 || a == 9) ? 100 : 100; } +/*member: constantFoldedControlFlow3:function(a) { + return a; +}*/ +bool constantFoldedControlFlow3(bool a) { + return a && 1 == 1 && 2 == 2; +} + +/*member: constantFoldedControlFlow4:function(a) { + return a; +}*/ +bool constantFoldedControlFlow4(bool a) { + return 1 == 1 && a && 2 == 2; +} + // Problem cases. // // Move the following cases above this comment when the code quality improves. @@ -130,29 +144,15 @@ bool constantFoldedControlFlow1(bool a, bool b) { } /*member: constantFoldedControlFlow2:function(a, b) { - return a && b && true; -}*/ -bool constantFoldedControlFlow2(bool a, bool b) { - return 1 == 1 && a && 2 == 2 && b && 3 == 3; -} - -/*member: constantFoldedControlFlow3:function(a) { var t1; if (a) - t1 = true; + t1 = b; else t1 = false; return t1; }*/ -bool constantFoldedControlFlow3(bool a) { - return a && 1 == 1 && 2 == 2; -} - -/*member: constantFoldedControlFlow4:function(a) { - return a && true; -}*/ -bool constantFoldedControlFlow4(bool a) { - return 1 == 1 && a && 2 == 2; +bool constantFoldedControlFlow2(bool a, bool b) { + return 1 == 1 && a && 2 == 2 && b && 3 == 3; } @pragma('dart2js:disable-inlining') diff --git a/pkg/compiler/test/codegen/data/phi_to_condition_test.dart b/pkg/compiler/test/codegen/data/phi_to_condition_test.dart new file mode 100644 index 00000000000..32af8ce446b --- /dev/null +++ b/pkg/compiler/test/codegen/data/phi_to_condition_test.dart @@ -0,0 +1,29 @@ +// 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:never-inline') +/*member: foo1:function(a, b) { + var changed = a !== b; + if (changed) + A.log("changed"); + return changed; +}*/ +foo1(int a, int b) { + bool changed = false; + if (a != b) { + changed = true; + log('changed'); + } + return changed; +} + +@pragma('dart2js:never-inline') +/*member: log:ignore*/ +void log(String s) {} + +/*member: main:ignore*/ +main() { + foo1(1, 2); + foo1(2, 1); +} diff --git a/tests/web/regress/issue/54115_loop_test.dart b/tests/web/regress/issue/54115_loop_test.dart new file mode 100644 index 00000000000..83641a0a489 --- /dev/null +++ b/tests/web/regress/issue/54115_loop_test.dart @@ -0,0 +1,29 @@ +// 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. + +var canSelect = false; +// Use late binding so the compiler doesn't inline and optimize out the bug. +late bool Function() select = () => true; + +void doWhileLoop() { + bool shouldRepeat; + do { + shouldRepeat = false; + if (canSelect) { + final isApproved = select(); + if (!isApproved) { + shouldRepeat = true; + continue; + } + } + } while (shouldRepeat); +} + +void main() { + canSelect = true; + doWhileLoop(); + + canSelect = false; + doWhileLoop(); +}