[vm/compiler] Fix IntConverter canonicalization
When going from Int64 through another representation back into Int64
you can discard a conversion if and only if it does not lose some
bits on the way. Previously we were checking for truncation flag, but
this is incorrect because it does not actually tell us anything about
about whether or not bits are going to be lost.
Instead, we should be checking the range of the source value.
In addition to fixing potential correctness issues, this CL actually
improves quality of code generated for Uint8 loads because it removes
a redundant chain of Int64->Uint32->Int64 conversions that follows
the load (which compiled 2 two redundant moves).
Issue https://github.com/dart-lang/sdk/issues/40308
TEST=vm/cc/IL_IntConverterCanonicalization
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-release-x64-try
Change-Id: Ifef0943a50d414bde41b2194e30b58ad3a8c106c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187921
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
index fb55f7d..ecf74ae 100644
--- a/runtime/vm/compiler/backend/il.cc
+++ b/runtime/vm/compiler/backend/il.cc
@@ -3389,16 +3389,18 @@
IntConverterInstr* box_defn = value()->definition()->AsIntConverter();
if ((box_defn != NULL) && (box_defn->representation() == from())) {
- // Do not erase truncating conversions from 64-bit value to 32-bit values
- // because such conversions erase upper 32 bits.
- if ((box_defn->from() == kUnboxedInt64) && box_defn->is_truncating()) {
+ // If the first convertion can erase bits (or deoptimize) we can't
+ // canonicalize it away.
+ auto src_defn = box_defn->value()->definition();
+ if ((box_defn->from() == kUnboxedInt64) &&
+ !Range::Fits(src_defn->range(), box_defn->to())) {
return this;
}
- // It's safe to discard any other conversions from and then back to the same
- // integer type.
+ // Otherise it is safe to discard any other conversions from and then back
+ // to the same integer type.
if (box_defn->from() == to()) {
- return box_defn->value()->definition();
+ return src_defn;
}
// Do not merge conversions where the first starts from Untagged or the
diff --git a/runtime/vm/compiler/backend/il_test.cc b/runtime/vm/compiler/backend/il_test.cc
index 4788686..e02bec5 100644
--- a/runtime/vm/compiler/backend/il_test.cc
+++ b/runtime/vm/compiler/backend/il_test.cc
@@ -7,7 +7,10 @@
#include <vector>
#include "platform/utils.h"
+#include "vm/compiler/backend/block_builder.h"
+#include "vm/compiler/backend/il_printer.h"
#include "vm/compiler/backend/il_test_helper.h"
+#include "vm/compiler/backend/range_analysis.h"
#include "vm/unit_test.h"
namespace dart {
@@ -189,4 +192,73 @@
expected_stores_aot);
}
+// Returns |true| if compiler canonicalizes away a chain of IntConverters going
+// from |initial| representation to |intermediate| representation and then
+// back to |initial| given that initial value has range [min_value, max_value].
+bool TestIntConverterCanonicalizationRule(Thread* thread,
+ int64_t min_value,
+ int64_t max_value,
+ Representation initial,
+ Representation intermediate) {
+ using compiler::BlockBuilder;
+
+ CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
+
+ FlowGraphBuilderHelper H;
+
+ // Add a variable into the scope which would provide static type for the
+ // parameter.
+ LocalVariable* v0_var =
+ new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
+ String::Handle(Symbols::New(thread, "v0")),
+ AbstractType::ZoneHandle(Type::IntType()));
+ v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
+ H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
+
+ auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
+
+ Definition* v0;
+ ReturnInstr* ret;
+
+ {
+ BlockBuilder builder(H.flow_graph(), normal_entry);
+ v0 = builder.AddParameter(0, 0, /*with_frame=*/true, initial);
+ v0->set_range(Range(RangeBoundary::FromConstant(min_value),
+ RangeBoundary::FromConstant(max_value)));
+ auto conv1 = builder.AddDefinition(new IntConverterInstr(
+ initial, intermediate, new Value(v0), S.GetNextDeoptId()));
+ auto conv2 = builder.AddDefinition(new IntConverterInstr(
+ intermediate, initial, new Value(conv1), S.GetNextDeoptId()));
+ ret = builder.AddReturn(new Value(conv2));
+ }
+
+ H.FinishGraph();
+
+ H.flow_graph()->Canonicalize();
+ H.flow_graph()->Canonicalize();
+
+ return ret->value()->definition() == v0;
+}
+
+ISOLATE_UNIT_TEST_CASE(IL_IntConverterCanonicalization) {
+ EXPECT(TestIntConverterCanonicalizationRule(thread, kMinInt16, kMaxInt16,
+ kUnboxedInt64, kUnboxedInt32));
+ EXPECT(TestIntConverterCanonicalizationRule(thread, kMinInt32, kMaxInt32,
+ kUnboxedInt64, kUnboxedInt32));
+ EXPECT(!TestIntConverterCanonicalizationRule(
+ thread, kMinInt32, static_cast<int64_t>(kMaxInt32) + 1, kUnboxedInt64,
+ kUnboxedInt32));
+ EXPECT(TestIntConverterCanonicalizationRule(thread, 0, kMaxInt16,
+ kUnboxedInt64, kUnboxedUint32));
+ EXPECT(TestIntConverterCanonicalizationRule(thread, 0, kMaxInt32,
+ kUnboxedInt64, kUnboxedUint32));
+ EXPECT(TestIntConverterCanonicalizationRule(thread, 0, kMaxUint32,
+ kUnboxedInt64, kUnboxedUint32));
+ EXPECT(!TestIntConverterCanonicalizationRule(
+ thread, 0, static_cast<int64_t>(kMaxUint32) + 1, kUnboxedInt64,
+ kUnboxedUint32));
+ EXPECT(!TestIntConverterCanonicalizationRule(thread, -1, kMaxInt16,
+ kUnboxedInt64, kUnboxedUint32));
+}
+
} // namespace dart
diff --git a/runtime/vm/compiler/backend/range_analysis.h b/runtime/vm/compiler/backend/range_analysis.h
index 9a8971e..7491def 100644
--- a/runtime/vm/compiler/backend/range_analysis.h
+++ b/runtime/vm/compiler/backend/range_analysis.h
@@ -439,6 +439,27 @@
!max().UpperBound().Overflowed(size);
}
+ // Returns true if this range fits without truncation into
+ // the given representation.
+ static bool Fits(Range* range, Representation rep) {
+ if (range == nullptr) return false;
+
+ switch (rep) {
+ case kUnboxedInt64:
+ return true;
+
+ case kUnboxedInt32:
+ return range->Fits(RangeBoundary::kRangeBoundaryInt32);
+
+ case kUnboxedUint32:
+ return range->IsWithin(0, kMaxUint32);
+
+ default:
+ break;
+ }
+ return false;
+ }
+
// Clamp this to be within size.
void Clamp(RangeBoundary::RangeSize size);
