[vm/compiler] Fix negated integer comparisons in AOT
Fixes https://github.com/flutter/flutter/issues/19612
Change-Id: I5cf45b7243e4c2b50bb29f8b0f3dafc55a1f1838
Reviewed-on: https://dart-review.googlesource.com/67040
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Aart Bik <ajcbik@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
diff --git a/runtime/vm/compiler/assembler/assembler.h b/runtime/vm/compiler/assembler/assembler.h
index bc16cba..3eddf5d 100644
--- a/runtime/vm/compiler/assembler/assembler.h
+++ b/runtime/vm/compiler/assembler/assembler.h
@@ -24,7 +24,7 @@
class AssemblerBuffer;
class MemoryRegion;
-class Label : public ValueObject {
+class Label : public ZoneAllocated {
public:
Label() : position_(0), unresolved_(0) {
#ifdef DEBUG
diff --git a/runtime/vm/compiler/backend/il_arm.cc b/runtime/vm/compiler/backend/il_arm.cc
index 36a33a2..7505abd 100644
--- a/runtime/vm/compiler/backend/il_arm.cc
+++ b/runtime/vm/compiler/backend/il_arm.cc
@@ -3372,6 +3372,12 @@
__ b(instruction()->is_negated() ? labels_.true_label
: labels_.false_label);
} else {
+ if (instruction()->is_negated()) {
+ // Need to negate the result of slow path call.
+ __ CompareObject(result, Bool::True());
+ __ LoadObject(result, Bool::True(), NE);
+ __ LoadObject(result, Bool::False(), EQ);
+ }
__ b(exit_label());
}
}
diff --git a/runtime/vm/compiler/backend/il_arm64.cc b/runtime/vm/compiler/backend/il_arm64.cc
index 6049bb7..5312520 100644
--- a/runtime/vm/compiler/backend/il_arm64.cc
+++ b/runtime/vm/compiler/backend/il_arm64.cc
@@ -3079,7 +3079,9 @@
EQ);
__ b(instruction()->is_negated() ? labels_.true_label
: labels_.false_label);
+ ASSERT(exit_label()->IsUnused());
} else {
+ ASSERT(!instruction()->is_negated());
__ b(exit_label());
}
}
@@ -3138,28 +3140,38 @@
Condition true_condition = EmitComparisonCode(compiler, labels);
ASSERT(true_condition != kInvalidCondition);
EmitBranchOnCondition(compiler, true_condition, labels);
- __ Bind(slow_path->exit_label());
+ // No need to bind slow_path->exit_label() as slow path exits through
+ // true/false branch labels.
}
void CheckedSmiComparisonInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
- Label true_label, false_label, done;
- BranchLabels labels = {&true_label, &false_label, &false_label};
+ // Zone-allocate labels to pass them to slow-path which outlives local scope.
+ Label* true_label = new (Z) Label();
+ Label* false_label = new (Z) Label();
+ Label done;
+ BranchLabels labels = {true_label, false_label, false_label};
+ // In case of negated comparison result of a slow path call should be negated.
+ // For this purpose, 'merged' slow path is generated: it tests
+ // result of a call and jumps directly to true or false label.
CheckedSmiComparisonSlowPath* slow_path = new CheckedSmiComparisonSlowPath(
this, compiler->CurrentTryIndex(), labels,
- /* merged = */ false);
+ /* merged = */ is_negated());
compiler->AddSlowPathCode(slow_path);
EMIT_SMI_CHECK;
Condition true_condition = EmitComparisonCode(compiler, labels);
ASSERT(true_condition != kInvalidCondition);
EmitBranchOnCondition(compiler, true_condition, labels);
Register result = locs()->out(0).reg();
- __ Bind(&false_label);
+ __ Bind(false_label);
__ LoadObject(result, Bool::False());
__ b(&done);
- __ Bind(&true_label);
+ __ Bind(true_label);
__ LoadObject(result, Bool::True());
__ Bind(&done);
- __ Bind(slow_path->exit_label());
+ // In case of negated comparison slow path exits through true/false labels.
+ if (!is_negated()) {
+ __ Bind(slow_path->exit_label());
+ }
}
LocationSummary* BinarySmiOpInstr::MakeLocationSummary(Zone* zone,
diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc
index 5c5658f..8365db6 100644
--- a/runtime/vm/compiler/backend/il_x64.cc
+++ b/runtime/vm/compiler/backend/il_x64.cc
@@ -2994,7 +2994,7 @@
CheckedSmiComparisonSlowPath(CheckedSmiComparisonInstr* instruction,
intptr_t try_index,
BranchLabels labels,
- bool merged = false)
+ bool merged)
: TemplateSlowPathCode(instruction),
try_index_(try_index),
labels_(labels),
@@ -3033,7 +3033,9 @@
: labels_.true_label);
__ jmp(instruction()->is_negated() ? labels_.true_label
: labels_.false_label);
+ ASSERT(exit_label()->IsUnused());
} else {
+ ASSERT(!instruction()->is_negated());
__ jmp(exit_label());
}
}
@@ -3093,28 +3095,38 @@
Condition true_condition = EmitComparisonCode(compiler, labels);
ASSERT(true_condition != INVALID_CONDITION);
EmitBranchOnCondition(compiler, true_condition, labels);
- __ Bind(slow_path->exit_label());
+ // No need to bind slow_path->exit_label() as slow path exits through
+ // true/false branch labels.
}
void CheckedSmiComparisonInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
- Label true_label, false_label, done;
- BranchLabels labels = {&true_label, &false_label, &false_label};
+ // Zone-allocate labels to pass them to slow-path which outlives local scope.
+ Label* true_label = new (Z) Label();
+ Label* false_label = new (Z) Label();
+ Label done;
+ BranchLabels labels = {true_label, false_label, false_label};
+ // In case of negated comparison result of a slow path call should be negated.
+ // For this purpose, 'merged' slow path is generated: it tests
+ // result of a call and jumps directly to true or false label.
CheckedSmiComparisonSlowPath* slow_path = new CheckedSmiComparisonSlowPath(
this, compiler->CurrentTryIndex(), labels,
- /* merged = */ false);
+ /* merged = */ is_negated());
compiler->AddSlowPathCode(slow_path);
EMIT_SMI_CHECK;
Condition true_condition = EmitComparisonCode(compiler, labels);
ASSERT(true_condition != INVALID_CONDITION);
EmitBranchOnCondition(compiler, true_condition, labels);
Register result = locs()->out(0).reg();
- __ Bind(&false_label);
+ __ Bind(false_label);
__ LoadObject(result, Bool::False());
__ jmp(&done);
- __ Bind(&true_label);
+ __ Bind(true_label);
__ LoadObject(result, Bool::True());
__ Bind(&done);
- __ Bind(slow_path->exit_label());
+ // In case of negated comparison slow path exits through true/false labels.
+ if (!is_negated()) {
+ __ Bind(slow_path->exit_label());
+ }
}
static bool CanBeImmediate(const Object& constant) {
diff --git a/tests/language_2/vm/regress_flutter_19612_test.dart b/tests/language_2/vm/regress_flutter_19612_test.dart
new file mode 100644
index 0000000..e7b3af1
--- /dev/null
+++ b/tests/language_2/vm/regress_flutter_19612_test.dart
@@ -0,0 +1,26 @@
+// Copyright (c) 2018, 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.
+
+// Regression test for https://github.com/flutter/flutter/issues/19612.
+// This test verifies that negated condition is correctly handled by the
+// AOT compiler.
+
+import "package:expect/expect.dart";
+
+class X {
+ final int maxLines;
+ X([this.maxLines]);
+
+ bool get isMultiline => maxLines != 1;
+}
+
+X x1 = new X(1);
+X x2 = new X(42);
+X x3 = new X();
+
+main() {
+ Expect.isFalse(x1.isMultiline);
+ Expect.isTrue(x2.isMultiline);
+ Expect.isTrue(x3.isMultiline);
+}
