[vm] Fix IntConverter canonicalization

If bits are reinterpreted or truncated, conversions must not be removed. TEST=vm/cc/IL_IntConverterCanonicalization Closes: dart-lang#53613 Change-Id: Iab6f1b0540caa782b4b09fef608817930583461d Cq-Include-Trybots: luci.dart.try:vm-linux-debug-ia32-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327865 Reviewed-by: Slava Egorov <[email protected]> Commit-Queue: Daco Harkes <[email protected]>
nguyencse · Oct 13, 2023 · de582bf · de582bf
1 parent 3e73f5b
commit de582bf
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 23 deletions.
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
@@ -3323,31 +3323,33 @@ Definition* IntConverterInstr::Canonicalize(FlowGraph* flow_graph) {
     }
   }
 
-  IntConverterInstr* box_defn = value()->definition()->AsIntConverter();
-  if ((box_defn != nullptr) && (box_defn->representation() == from())) {
-    // If the first conversion 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())) {
+  // Fold IntCoverter(b->c, IntConverter(a->b, v)) into IntConverter(a->c, v).
+  IntConverterInstr* first_converter = value()->definition()->AsIntConverter();
+  if ((first_converter != nullptr) &&
+      (first_converter->representation() == from())) {
+    const auto intermediate_rep = first_converter->representation();
+    // Only eliminate intermediate conversion if it does not change the value.
+    auto src_defn = first_converter->value()->definition();
+    if (!Range::Fits(src_defn->range(), intermediate_rep)) {
       return this;
     }
 
     // Otherise it is safe to discard any other conversions from and then back
     // to the same integer type.
-    if (box_defn->from() == to()) {
+    if (first_converter->from() == to()) {
       return src_defn;
     }
 
     // Do not merge conversions where the first starts from Untagged or the
     // second ends at Untagged, since we expect to see either UnboxedIntPtr
     // or UnboxedFfiIntPtr as the other type in an Untagged conversion.
-    if ((box_defn->from() == kUntagged) || (to() == kUntagged)) {
+    if ((first_converter->from() == kUntagged) || (to() == kUntagged)) {
       return this;
     }
 
     IntConverterInstr* converter = new IntConverterInstr(
-        box_defn->from(), representation(), box_defn->value()->CopyWithType(),
+        first_converter->from(), representation(),
+        first_converter->value()->CopyWithType(),
         (to() == kUnboxedInt32) ? GetDeoptId() : DeoptId::kNone);
     if ((representation() == kUnboxedInt32) && is_truncating()) {
       converter->mark_truncating();

diff --git a/runtime/vm/compiler/backend/il_test.cc b/runtime/vm/compiler/backend/il_test.cc
@@ -202,7 +202,8 @@ bool TestIntConverterCanonicalizationRule(Thread* thread,
                                           int64_t min_value,
                                           int64_t max_value,
                                           Representation initial,
-                                          Representation intermediate) {
+                                          Representation intermediate,
+                                          Representation final) {
   using compiler::BlockBuilder;
 
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
@@ -237,23 +238,37 @@ bool TestIntConverterCanonicalizationRule(Thread* thread,
 
 ISOLATE_UNIT_TEST_CASE(IL_IntConverterCanonicalization) {
   EXPECT(TestIntConverterCanonicalizationRule(thread, kMinInt16, kMaxInt16,
-                                              kUnboxedInt64, kUnboxedInt32));
+                                              kUnboxedInt64, kUnboxedInt32,
+                                              kUnboxedInt64));
   EXPECT(TestIntConverterCanonicalizationRule(thread, kMinInt32, kMaxInt32,
-                                              kUnboxedInt64, kUnboxedInt32));
+                                              kUnboxedInt64, kUnboxedInt32,
+                                              kUnboxedInt64));
   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));
+      kUnboxedInt32, kUnboxedInt64));
+  EXPECT(TestIntConverterCanonicalizationRule(
+      thread, 0, kMaxInt16, kUnboxedInt64, kUnboxedUint32, kUnboxedInt64));
+  EXPECT(TestIntConverterCanonicalizationRule(
+      thread, 0, kMaxInt32, kUnboxedInt64, kUnboxedUint32, kUnboxedInt64));
+  EXPECT(TestIntConverterCanonicalizationRule(
+      thread, 0, kMaxUint32, kUnboxedInt64, kUnboxedUint32, kUnboxedInt64));
   EXPECT(!TestIntConverterCanonicalizationRule(
       thread, 0, static_cast<int64_t>(kMaxUint32) + 1, kUnboxedInt64,
-      kUnboxedUint32));
-  EXPECT(!TestIntConverterCanonicalizationRule(thread, -1, kMaxInt16,
-                                               kUnboxedInt64, kUnboxedUint32));
+      kUnboxedUint32, kUnboxedInt64));
+  EXPECT(!TestIntConverterCanonicalizationRule(
+      thread, -1, kMaxInt16, kUnboxedInt64, kUnboxedUint32, kUnboxedInt64));
+
+  // Regression test for https://dartbug.com/53613.
+  EXPECT(!TestIntConverterCanonicalizationRule(thread, kMinInt32, kMaxInt32,
+                                               kUnboxedInt32, kUnboxedUint32,
+                                               kUnboxedInt64));
+  EXPECT(!TestIntConverterCanonicalizationRule(thread, kMinInt32, kMaxInt32,
+                                               kUnboxedInt32, kUnboxedUint32,
+                                               kUnboxedInt32));
+  EXPECT(TestIntConverterCanonicalizationRule(
+      thread, 0, kMaxInt32, kUnboxedInt32, kUnboxedUint32, kUnboxedInt64));
+  EXPECT(TestIntConverterCanonicalizationRule(
+      thread, 0, kMaxInt32, kUnboxedInt32, kUnboxedUint32, kUnboxedInt32));
 }
 
 ISOLATE_UNIT_TEST_CASE(IL_PhiCanonicalization) {