[vm/compiler] Force unboxing of int phis on 64-bit platforms

More specifically on 64-bit platforms, when other heuristic fail:

- in AOT mode unbox all int typed phis.
- in JIT mode unbox all non-Smi int typed phis i.e. mixed
  Mint/Smi phis.

This brings treatment of int phis closer to how we treat other
boxed phis.

This considerably improves AOT code quality for loops which
our current conservative heuristic does not currently hit,
for example:

  void foo(int start, int end) {
    for (var i = start; i < end; i++) {
    }
  }

Code like this would use a boxed Phi previously which does not make
sense.

In JIT mode to really benefit from aggressive unboxing we
might need to shift from over-reliance on Smi operations, e.g.
when phi is unboxed we want to cascade that through and unbox
operations which are performed on it transitively. This requires
much more sophisticated algorithm and is outside of scope for this
change.

In AOT we in general treat all arithmetic operations as operations
over 64-bit int values so unboxing phis makes more sense.

As a result of this change we see small improvement in code size:

  flutter_gallery_app_so_gzip_size -0.2392%

TEST=ci


Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-release-x64-try
Change-Id: Iae9a16f353e53f649da4e665d2812ad47099a674
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187860
Commit-Queue: Vyacheslav Egorov <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>

Author: mraleph

runtime/vm/compiler/backend/constant_propagator_test.cc

@@ -51,7 +51,7 @@ ISOLATE_UNIT_TEST_CASE(ConstantPropagation_PhiUnwrappingAndConvergence) {
 
   {
     BlockBuilder builder(H.flow_graph(), b2);
-    v1 = H.Phi(b2, {{b1, v0}, {b3, FlowGraphBuilderHelper::kPhiSelfReference}});
+    v1 = H.Phi(b2, {{b1, v0}, {b3, &v1}});
     builder.AddPhi(v1);
     auto v2 = builder.AddDefinition(
         new EqualityCompareInstr(InstructionSource(), Token::kEQ, new Value(v1),

runtime/vm/compiler/backend/flow_graph.cc

@@ -1901,7 +1901,7 @@ void FlowGraph::InsertConversionsFor(Definition* def) {
   }
 }
 
-static void UnboxPhi(PhiInstr* phi) {
+static void UnboxPhi(PhiInstr* phi, bool is_aot) {
   Representation unboxed = phi->representation();
 
   switch (phi->Type()->ToCid()) {
@@ -2004,10 +2004,22 @@ static void UnboxPhi(PhiInstr* phi) {
     }
   }
 
+#if defined(TARGET_ARCH_IS_64_BIT)
+  // In AOT mode on 64-bit platforms always unbox integer typed phis (similar
+  // to how we treat doubles and other boxed numeric types).
+  // In JIT mode only unbox phis which are not fully known to be Smi.
+  if ((unboxed == kTagged) && phi->Type()->IsInt() &&
+      (is_aot || phi->Type()->ToCid() != kSmiCid)) {
+    unboxed = kUnboxedInt64;
+  }
+#endif
+
   phi->set_representation(unboxed);
 }
 
 void FlowGraph::SelectRepresentations() {
+  const auto is_aot = CompilerState::Current().is_aot();
+
   // First we decide for each phi if it is beneficial to unbox it. If so, we
   // change it's `phi->representation()`
   for (BlockIterator block_it = reverse_postorder_iterator(); !block_it.Done();
@@ -2016,7 +2028,7 @@ void FlowGraph::SelectRepresentations() {
     if (join_entry != NULL) {
       for (PhiIterator it(join_entry); !it.Done(); it.Advance()) {
         PhiInstr* phi = it.Current();
-        UnboxPhi(phi);
+        UnboxPhi(phi, is_aot);
       }
     }
   }

runtime/vm/compiler/backend/flow_graph_test.cc

@@ -0,0 +1,85 @@
+// Copyright (c) 2012, 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.
+
+#include "vm/compiler/backend/flow_graph.h"
+
+#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/type_propagator.h"
+#include "vm/unit_test.h"
+
+namespace dart {
+
+#if defined(TARGET_ARCH_IS_64_BIT)
+ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxInt64Phi) {
+  using compiler::BlockBuilder;
+
+  CompilerState S(thread, /*is_aot=*/true, /*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()),
+                        new CompileType(CompileType::Int()));
+  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();
+  auto loop_header = H.JoinEntry();
+  auto loop_body = H.TargetEntry();
+  auto loop_exit = H.TargetEntry();
+
+  Definition* v0;
+  PhiInstr* loop_var;
+  Definition* add1;
+  ReturnInstr* ret;
+
+  {
+    BlockBuilder builder(H.flow_graph(), normal_entry);
+    v0 = builder.AddParameter(0, 0, /*with_frame=*/true, kTagged);
+    builder.AddInstruction(new GotoInstr(loop_header, S.GetNextDeoptId()));
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), loop_header);
+    loop_var = H.Phi(loop_header, {{normal_entry, v0}, {loop_body, &add1}});
+    builder.AddPhi(loop_var);
+    builder.AddBranch(new RelationalOpInstr(
+                          InstructionSource(), Token::kLT, new Value(loop_var),
+                          new Value(H.IntConstant(50)), kMintCid,
+                          S.GetNextDeoptId(), Instruction::kNotSpeculative),
+                      loop_body, loop_exit);
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), loop_body);
+    add1 = builder.AddDefinition(new BinaryInt64OpInstr(
+        Token::kADD, new Value(loop_var), new Value(H.IntConstant(1)),
+        S.GetNextDeoptId(), Instruction::kNotSpeculative));
+    builder.AddInstruction(new GotoInstr(loop_header, S.GetNextDeoptId()));
+  }
+
+  {
+    BlockBuilder builder(H.flow_graph(), loop_exit);
+    ret = builder.AddReturn(new Value(loop_var));
+  }
+
+  H.FinishGraph();
+
+  FlowGraphTypePropagator::Propagate(H.flow_graph());
+  H.flow_graph()->SelectRepresentations();
+
+  EXPECT_PROPERTY(loop_var, it.representation() == kUnboxedInt64);
+}
+#endif  // defined(TARGET_ARCH_IS_64_BIT)
+
+}  // namespace dart

runtime/vm/compiler/backend/il_test_helper.cc

@@ -21,8 +21,6 @@
 
 namespace dart {
 
-Definition* const FlowGraphBuilderHelper::kPhiSelfReference = nullptr;
-
 LibraryPtr LoadTestScript(const char* script,
                           Dart_NativeEntryResolver resolver,
                           const char* lib_uri) {

runtime/vm/compiler/backend/il_test_helper.h

@@ -5,6 +5,7 @@
 #ifndef RUNTIME_VM_COMPILER_BACKEND_IL_TEST_HELPER_H_
 #define RUNTIME_VM_COMPILER_BACKEND_IL_TEST_HELPER_H_
 
+#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -277,20 +278,48 @@ class FlowGraphBuilderHelper {
     return flow_graph_.GetConstant(Double::Handle(Double::NewCanonical(value)));
   }
 
-  static Definition* const kPhiSelfReference;
+  enum class IncomingDefKind {
+    kImmediate,
+    kDelayed,
+  };
+
+  class IncomingDef {
+   public:
+    IncomingDef(BlockEntryInstr* from, Definition* defn)
+        : kind_(IncomingDefKind::kImmediate), from_(from), defn_(defn) {}
+
+    template <typename T,
+              typename = typename std::enable_if<
+                  std::is_base_of<Definition, T>::value>::type>
+    IncomingDef(BlockEntryInstr* from, T** defn_source)
+        : kind_(IncomingDefKind::kDelayed),
+          from_(from),
+          defn_source_(reinterpret_cast<Definition**>(defn_source)) {}
+
+    BlockEntryInstr* from() const { return from_; }
+    Definition* defn() const {
+      return kind_ == IncomingDefKind::kImmediate ? defn_ : *defn_source_;
+    }
+
+   private:
+    IncomingDefKind kind_;
+    BlockEntryInstr* from_;
+    union {
+      Definition* defn_;
+      Definition** defn_source_;
+    };
+  };
 
   PhiInstr* Phi(JoinEntryInstr* join,
-                std::initializer_list<std::pair<BlockEntryInstr*, Definition*>>
-                    incoming) {
+                std::initializer_list<IncomingDef> incoming) {
     auto phi = new PhiInstr(join, incoming.size());
     for (size_t i = 0; i < incoming.size(); i++) {
       auto input = new Value(flow_graph_.constant_dead());
       phi->SetInputAt(i, input);
       input->definition()->AddInputUse(input);
     }
-    for (auto pair : incoming) {
-      pending_phis_.Add({phi, pair.first,
-                         pair.second == kPhiSelfReference ? phi : pair.second});
+    for (auto def : incoming) {
+      pending_phis_.Add({phi, def});
     }
     return phi;
   }
@@ -303,9 +332,9 @@ class FlowGraphBuilderHelper {
     for (auto& pending : pending_phis_) {
       auto join = pending.phi->block();
       EXPECT(pending.phi->InputCount() == join->PredecessorCount());
-      auto pred_index = join->IndexOfPredecessor(pending.pred);
+      auto pred_index = join->IndexOfPredecessor(pending.incoming.from());
       EXPECT(pred_index != -1);
-      pending.phi->InputAt(pred_index)->BindTo(pending.defn);
+      pending.phi->InputAt(pred_index)->BindTo(pending.incoming.defn());
     }
   }
 
@@ -347,8 +376,7 @@ class FlowGraphBuilderHelper {
 
   struct PendingPhiInput {
     PhiInstr* phi;
-    BlockEntryInstr* pred;
-    Definition* defn;
+    IncomingDef incoming;
   };
   GrowableArray<PendingPhiInput> pending_phis_;
 };

runtime/vm/compiler/backend/inliner_test.cc

@@ -222,8 +222,8 @@ ISOLATE_UNIT_TEST_CASE(Inliner_List_generate) {
   RELEASE_ASSERT(cursor.TryMatch({
       kMoveGlob,
       kMatchAndMoveCreateArray,
-      kMatchAndMoveUnboxInt64,
       {kMoveAny, &unbox1},
+      kMatchAndMoveUnboxInt64,
       {kMoveAny, &unbox2},
       kMatchAndMoveGoto,
 

runtime/vm/compiler/backend/loops_test.cc

@@ -68,9 +68,13 @@ static const char* ComputeInduction(Thread* thread, const char* script_chars) {
   Invoke(root_library, "main");
 
   std::initializer_list<CompilerPass::Id> passes = {
-      CompilerPass::kComputeSSA,      CompilerPass::kTypePropagation,
-      CompilerPass::kApplyICData,     CompilerPass::kSelectRepresentations,
-      CompilerPass::kTypePropagation, CompilerPass::kCanonicalize,
+      CompilerPass::kComputeSSA,
+      CompilerPass::kTypePropagation,
+      CompilerPass::kApplyICData,
+      CompilerPass::kTypePropagation,
+      CompilerPass::kSelectRepresentations,
+      CompilerPass::kTypePropagation,
+      CompilerPass::kCanonicalize,
   };
   const auto& function = Function::Handle(GetFunction(root_library, "foo"));
   TestPipeline pipeline(function, CompilerPass::kJIT);
@@ -300,7 +304,7 @@ ISOLATE_UNIT_TEST_CASE(WrapAroundAndDerived) {
   const char* expected =
       "  [0\n"
       "  WRAP(99, LIN(0 + 1 * i))\n"    // phi (w)
-      "  LIN(0 + 1 * i) 100\n"          // phi
+      "  LIN(0 + 1 * i) 100\n"          // phi (i)
       "  WRAP(102, LIN(3 + 1 * i))\n"   // a
       "  WRAP(94, LIN(-5 + 1 * i))\n"   // b
       "  WRAP(693, LIN(0 + 7 * i))\n"   // c
@@ -383,11 +387,15 @@ ISOLATE_UNIT_TEST_CASE(NonStrictConditionUpWrap) {
   const char* expected =
       "  [0\n"
       "  LIN(9223372036854775806 + 1 * i)\n"  // phi
+#if !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(9223372036854775806 + 1 * i)\n"  // (un)boxing
       "  LIN(9223372036854775806 + 1 * i)\n"
       "  LIN(9223372036854775806 + 1 * i)\n"
+#endif                                        // !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(9223372036854775807 + 1 * i)\n"  // add
+#if !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(9223372036854775807 + 1 * i)\n"  // unbox
+#endif                                        // !defined(TARGET_ARCH_IS_64_BIT)
       "  ]\n";
   EXPECT_STREQ(expected, ComputeInduction(thread, script_chars));
 }
@@ -426,11 +434,15 @@ ISOLATE_UNIT_TEST_CASE(NonStrictConditionDownWrap) {
   const char* expected =
       "  [0\n"
       "  LIN(-9223372036854775807 + -1 * i)\n"  // phi
+#if !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(-9223372036854775807 + -1 * i)\n"  // (un)boxing
       "  LIN(-9223372036854775807 + -1 * i)\n"
       "  LIN(-9223372036854775807 + -1 * i)\n"
+#endif  // !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(-9223372036854775808 + -1 * i)\n"  // sub
+#if !defined(TARGET_ARCH_IS_64_BIT)
       "  LIN(-9223372036854775808 + -1 * i)\n"  // unbox
+#endif  // !defined(TARGET_ARCH_IS_64_BIT)
       "  ]\n";
   EXPECT_STREQ(expected, ComputeInduction(thread, script_chars));
 }

runtime/vm/compiler/compiler_sources.gni

@@ -160,6 +160,7 @@ compiler_sources_tests = [
   "assembler/disassembler_test.cc",
   "backend/bce_test.cc",
   "backend/constant_propagator_test.cc",
+  "backend/flow_graph_test.cc",
   "backend/il_test.cc",
   "backend/il_test_helper.h",
   "backend/il_test_helper.cc",