[vm/compiler] Choose correct unboxed float representation for phi ins…

…tructions. BUG=#50622 TEST=FlowGraph_UnboxedFloatPhi Change-Id: Ie7bfbb6c4028907a18243d44be8128999c1efcae Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/275180 Reviewed-by: Slava Egorov <[email protected]> Commit-Queue: Alexander Aprelev <[email protected]>
dart-lang · Dec 15, 2022 · ff2d661 · ff2d661
1 parent ca03588
commit ff2d661
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 4 deletions.
diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc
@@ -2053,7 +2053,16 @@ class PhiUnboxingHeuristic : public ValueObject {
     switch (phi->Type()->ToCid()) {
       case kDoubleCid:
         if (CanUnboxDouble()) {
-          unboxed = kUnboxedDouble;
+          // Could be UnboxedDouble or UnboxedFloat
+          unboxed = DetermineIfAnyIncomingUnboxedFloats(phi) ? kUnboxedFloat
+                                                             : kUnboxedDouble;
+#if defined(DEBUG)
+          if (unboxed == kUnboxedFloat) {
+            for (auto input : phi->inputs()) {
+              ASSERT(input->representation() != kUnboxedDouble);
+            }
+          }
+#endif
         }
         break;
       case kFloat32x4Cid:
@@ -2117,10 +2126,10 @@ class PhiUnboxingHeuristic : public ValueObject {
       // unboxed operation prefer to keep it unboxed.
       // We use this heuristic instead of eagerly unboxing all the phis
       // because we are concerned about the code size and register pressure.
-      const bool has_unboxed_incomming_value = HasUnboxedIncommingValue(phi);
+      const bool has_unboxed_incoming_value = HasUnboxedIncomingValue(phi);
       const bool flows_into_unboxed_use = FlowsIntoUnboxedUse(phi);
 
-      if (has_unboxed_incomming_value && flows_into_unboxed_use) {
+      if (has_unboxed_incoming_value && flows_into_unboxed_use) {
         unboxed =
             RangeUtils::Fits(phi->range(), RangeBoundary::kRangeBoundaryInt32)
                 ? kUnboxedInt32
@@ -2133,10 +2142,30 @@ class PhiUnboxingHeuristic : public ValueObject {
   }
 
  private:
+  // Returns [true] if there are UnboxedFloats representation flowing into
+  // the |phi|.
+  // This function looks through phis.
+  bool DetermineIfAnyIncomingUnboxedFloats(PhiInstr* phi) {
+    worklist_.Clear();
+    worklist_.Add(phi);
+    for (intptr_t i = 0; i < worklist_.definitions().length(); i++) {
+      const auto defn = worklist_.definitions()[i];
+      for (auto input : defn->inputs()) {
+        if (input->representation() == kUnboxedFloat) {
+          return true;
+        }
+        if (input->IsPhi()) {
+          worklist_.Add(input);
+        }
+      }
+    }
+    return false;
+  }
+
   // Returns |true| iff there is an unboxed definition among all potential
   // definitions that can flow into the |phi|.
   // This function looks through phis.
-  bool HasUnboxedIncommingValue(PhiInstr* phi) {
+  bool HasUnboxedIncomingValue(PhiInstr* phi) {
     worklist_.Clear();
     worklist_.Add(phi);
     for (intptr_t i = 0; i < worklist_.definitions().length(); i++) {

diff --git a/runtime/vm/compiler/backend/flow_graph_test.cc b/runtime/vm/compiler/backend/flow_graph_test.cc
@@ -140,6 +140,64 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_LateVariablePhiUnboxing) {
   EXPECT_PROPERTY(late_var, it.representation() == kTagged);
 }
 
+ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxedFloatPhi) {
+  using compiler::BlockBuilder;
+
+  CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
+  FlowGraphBuilderHelper H;
+
+  auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
+  auto then_body = H.TargetEntry();
+  auto else_body = H.TargetEntry();
+  auto join_exit = H.JoinEntry();
+
+  PhiInstr* phi;
+  Definition* double_to_float_1;
+  Definition* double_to_float_2;
+
+  {
+    BlockBuilder builder(H.flow_graph(), normal_entry);
+    builder.AddBranch(
+        new StrictCompareInstr(
+            InstructionSource(), Token::kEQ_STRICT, new Value(H.IntConstant(1)),
+            new Value(H.IntConstant(1)),
+            /*needs_number_check=*/false, S.GetNextDeoptId()),
+        then_body, else_body);
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), then_body);
+    double_to_float_1 = builder.AddDefinition(new DoubleToFloatInstr(
+        new Value(H.DoubleConstant(1)), S.GetNextDeoptId(),
+        Instruction::kNotSpeculative));
+    builder.AddInstruction(new GotoInstr(join_exit, S.GetNextDeoptId()));
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), else_body);
+    double_to_float_2 = builder.AddDefinition(new DoubleToFloatInstr(
+        new Value(H.DoubleConstant(2)), S.GetNextDeoptId(),
+        Instruction::kNotSpeculative));
+    builder.AddInstruction(new GotoInstr(join_exit, S.GetNextDeoptId()));
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), join_exit);
+    phi = new PhiInstr(join_exit, 3);
+    phi->SetInputAt(0, new Value(double_to_float_1));
+    phi->SetInputAt(1, new Value(double_to_float_2));
+    phi->SetInputAt(2, new Value(phi));
+    builder.AddPhi(phi);
+    builder.AddReturn(new Value(phi));
+  }
+  H.FinishGraph();
+
+  FlowGraphTypePropagator::Propagate(H.flow_graph());
+  H.flow_graph()->SelectRepresentations();
+
+  EXPECT_PROPERTY(phi, it.representation() == kUnboxedFloat);
+}
+
 void TestLargeFrame(const char* type,
                     const char* zero,
                     const char* one,