From 8bb16e2621130cfef5812249258bf5eee66a3932 Mon Sep 17 00:00:00 2001
From: Nikolai Tillmann <nikolait@meta.com>
Date: Fri, 12 Apr 2024 12:30:45 -0700
Subject: [PATCH] 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
---
 .../ObjectEscapeAnalysis.cpp                  | 39 +++++++------------
 test/integ/ObjectEscapeAnalysisTest.cpp       | 25 ++++++++++++
 test/integ/ObjectEscapeAnalysisTest.java      | 14 +++++++
 3 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/opt/object-escape-analysis/ObjectEscapeAnalysis.cpp b/opt/object-escape-analysis/ObjectEscapeAnalysis.cpp
index 322cd19415d..044cff5d0b4 100644
--- 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
index 62f14fa15f5..f09f3858355 100644
--- 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
index c5df7b327ab..93949857b90 100644
--- 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;