Fix initialization of optionally inlined objects in presence of loops

Summary: Instead of trying to be clever with the placement of the initialization code for optionally inlined objects in the presence of loops, just place the very initial zero-assignment and it at the beginning of the method body. Reviewed By: ssj933 Differential Revision: D55992980 fbshipit-source-id: d0eb0d05acf863e456e6158c9c9cbeb24cff2eca
kami-lang · Apr 12, 2024 · 8bb16e2 · 8bb16e2
1 parent 22fcf73
commit 8bb16e2
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 24 deletions.
diff --git a/opt/object-escape-analysis/ObjectEscapeAnalysis.cpp b/opt/object-escape-analysis/ObjectEscapeAnalysis.cpp
@@ -48,7 +48,6 @@
 #include "ApiLevelChecker.h"
 #include "CFGMutation.h"
 #include "ConfigFiles.h"
-#include "Dominators.h"
 #include "ExpandableMethodParams.h"
 #include "IRInstruction.h"
 #include "Inliner.h"
@@ -1352,10 +1351,6 @@ class RootMethodReducer {
       used_insns.insert(use.insn);
     }
 
-    auto doms = dominators::SimpleFastDominators<cfg::GraphInterface>(cfg);
-    cfg::Block* dom = nullptr;
-    auto refine_dom = [&](auto* b) { dom = dom ? doms.intersect(dom, b) : b; };
-
     cfg::CFGMutation mutation(cfg);
     auto ii = InstructionIterable(cfg);
     auto new_instance_insn_it = ii.end();
@@ -1371,7 +1366,6 @@ class RootMethodReducer {
         }
         continue;
       }
-      refine_dom(it.block());
       if (opcode::is_move_object(opcode)) {
         auto new_insn = (new IRInstruction(OPCODE_MOVE))
                             ->set_src(0, get_created_reg(insn->src(0)))
@@ -1446,16 +1440,16 @@ class RootMethodReducer {
     auto new_instance_move_result_it = cfg.move_result_of(new_instance_insn_it);
     always_assert(!new_instance_move_result_it.is_end());
 
-    auto insert_inits_before = [&](const auto& it, bool created) {
-      std::vector<IRInstruction*> inits;
-      inits.reserve(ordered_fields.size() + created_regs.size());
+    auto get_init_insns = [&](bool created) {
+      std::vector<IRInstruction*> insns;
+      insns.reserve(ordered_fields.size() + created_regs.size());
       for (auto field : ordered_fields) {
         auto wide = type::is_wide_type(field->get_type());
         auto opcode = wide ? OPCODE_CONST_WIDE : OPCODE_CONST;
         auto reg = field_regs.at(field);
         auto new_insn =
             (new IRInstruction(opcode))->set_literal(0)->set_dest(reg);
-        inits.push_back(new_insn);
+        insns.push_back(new_insn);
       }
       std::vector<reg_t> ordered_created_regs;
       ordered_created_regs.reserve(created_regs.size());
@@ -1470,23 +1464,20 @@ class RootMethodReducer {
                 ->set_literal(created &&
                               reg == new_instance_move_result_it->insn->dest())
                 ->set_dest(created_reg);
-        inits.push_back(new_insn);
+        insns.push_back(new_insn);
       }
-      mutation.insert_before(it, inits);
+      return insns;
     };
-    insert_inits_before(new_instance_insn_it, /* created */ true);
-
-    refine_dom(new_instance_insn_it.block());
-    always_assert(dom);
-    if (dom != new_instance_insn_it.block()) {
-      auto it = dom->to_cfg_instruction_iterator(
-          dom->get_first_non_param_loading_insn());
-      if (dom->starts_with_move_result() || dom->starts_with_move_exception()) {
-        it++;
-      }
-      insert_inits_before(it, /* created */ false);
-    }
+    mutation.insert_before(new_instance_insn_it,
+                           get_init_insns(/* created */ true));
+
     mutation.flush();
+
+    auto entry_block = cfg.entry_block();
+    auto entry_it = entry_block->to_cfg_instruction_iterator(
+        entry_block->get_first_non_param_loading_insn());
+    cfg.insert_before(entry_it, get_init_insns(/* created */ false));
+
     // simplify to prune now unreachable code, e.g. from removed exception
     // handlers
     cfg.simplify();

diff --git a/test/integ/ObjectEscapeAnalysisTest.cpp b/test/integ/ObjectEscapeAnalysisTest.cpp
@@ -375,6 +375,31 @@ TEST_F(ObjectEscapeAnalysisTest, optionalReduceToBC) {
   ASSERT_EQ(actual.str(), assembler::to_s_expr(expected.get()).str());
 }
 
+TEST_F(ObjectEscapeAnalysisTest, optionalLoopyReduceTo42) {
+  run();
+
+  auto actual = get_s_expr(
+      "Lcom/facebook/redextest/"
+      "ObjectEscapeAnalysisTest;.optionalLoopyReduceTo42:()I");
+  auto expected = assembler::ircode_from_string(R"(
+   (
+      (const v11 0)
+      (const v0 0)
+      (:L0)
+      (if-eqz v0 :L2)
+      (const v2 2)
+      (if-ne v0 v2 :L1)
+      (return v11)
+      (:L1)
+      (const v11 42)
+      (:L2)
+      (add-int/lit v0 v0 1)
+      (goto :L0)
+    )
+)");
+  ASSERT_EQ(actual.str(), assembler::to_s_expr(expected.get()).str());
+}
+
 TEST_F(ObjectEscapeAnalysisTest, objectIsNotNull) {
   run();
 

diff --git a/test/integ/ObjectEscapeAnalysisTest.java b/test/integ/ObjectEscapeAnalysisTest.java
@@ -251,6 +251,20 @@ public static boolean optionalReduceToBC(boolean b, boolean c) {
     return j instanceof Object;
   }
 
+  public static int optionalLoopyReduceTo42() {
+    I i = null;
+    int c = 0;
+    while (true) {
+      if (c != 0) {
+        if (c == 2) {
+          return i.getX();
+        }
+        i = I.allocator(42);
+      }
+      c++;
+    }
+  }
+
   public static boolean objectIsNotNull() {
     I i = I.allocator(42);
     return i == null;