Replace removeUnreachableBlocks with a targeted inliner fix

Summary:
D110056992 fixed a JIT crash when inlining a callee with no reachable `return`
by running `removeUnreachableBlocks` over the whole caller after the inlining
loop.  This replaces that with a more targeted fix at the point where the bad IR
is produced.

When `HIRBuilder::inlineHIR` merges the callee's returns into a single exit
block, a callee with no reachable `return` leaves that exit block with no
predecessors, so nothing flows into the merged `return_val`.  We now define
`return_val` with a `LoadConst<Bottom>` in that case, so the inliner's `Return`
-> `Assign`+`Branch` rewrite never references an undefined register.  The
resulting dead blocks are dropped by the `CleanCFG` that already runs at the end
of `InlineFunctionCalls::Run`, so the explicit `removeUnreachableBlocks(irfunc)`
call is no longer needed.

Reviewed By: mpage

Differential Revision: D110145444

fbshipit-source-id: 9260014528d9f0b41ac37bef10ec8222c4847b65

Author: alexmalyshev

cinderx/Jit/hir/builder.cpp

@@ -732,26 +732,37 @@ InlineResult HIRBuilder::inlineHIR(
   checkTranslate();
 
   BasicBlock* entry_block = buildHIRImpl(caller, caller_frame_state);
+
   // Make one block with a Return that merges the return branches from the
-  // callee. After SSA, it will turn into a massive Phi. The caller can find
+  // callee.  After SSA, it will turn into a massive Phi.  The caller can find
   // the Return and use it as the output of the call instruction.
   Register* return_val = caller->env.AllocateRegister();
   BasicBlock* exit_block = caller->cfg.AllocateBlock();
-  if (preloader_.returnType() <= TPrimitive) {
-    exit_block->append<Return>(return_val, preloader_.returnType());
-  } else {
-    exit_block->append<Return>(return_val);
-  }
+  size_t num_preds = 0;
   for (auto block : caller->cfg.GetRPOTraversal(entry_block)) {
     auto instr = block->GetTerminator();
     if (instr->IsReturn()) {
       auto assign = Assign::create(return_val, instr->GetOperand(0));
       auto branch = Branch::create(exit_block);
       instr->ExpandInto({assign, branch});
       delete instr;
+      num_preds += 1;
     }
   }
 
+  // If the callee has no reachable return then the exit block is unreachable
+  // and nothing flows into `return_val`.  This will wreak havoc with later
+  // optimization passes as they can't handle an undefined value, so define it
+  // as Bottom.
+  if (num_preds == 0) {
+    exit_block->append<LoadConst>(return_val, TBottom);
+  }
+  if (preloader_.returnType() <= TPrimitive) {
+    exit_block->append<Return>(return_val, preloader_.returnType());
+  } else {
+    exit_block->append<Return>(return_val);
+  }
+
   // Map of FrameState to parent pointers. We must completely disconnect the
   // inlined function's CFG from its caller for SSAify to run properly: it will
   // find uses (in FrameState) before defs and insert LoadConst<Nullptr>.

cinderx/Jit/hir/inliner.cpp

@@ -280,7 +280,7 @@ std::optional<InlineResult> inlineFunctionCall(
   }
   tail->push_front(EndInlinedFunction::create(begin_inlined_function));
 
-  // Transform LoadArg into Assign
+  // Transform LoadArg into Assign.  They'll only be in the entry block.
   for (auto it = result.entry->begin(); it != result.entry->end();) {
     auto& instr = *it;
     ++it;
@@ -294,7 +294,8 @@ std::optional<InlineResult> inlineFunctionCall(
     }
   }
 
-  // Transform Return into Assign+Branch
+  // Transform Return into Assign+Branch.  The HIRBuilder guarantees that the
+  // callee's exit block always has a Return, even if it is unreachable.
   auto return_instr = result.exit->GetTerminator();
   JIT_CHECK(
       return_instr->IsReturn(),
@@ -603,12 +604,11 @@ void InlineFunctionCalls::Run(Function& irfunc) {
     enqueueCandidates(call_code, funcFullname(call.func), nested);
   }
 
-  // The inliner will make some blocks unreachable and we need to remove them to
-  // make the CFG valid again.  While inlining might make some blocks
-  // unreachable and therefore make less work (less to inline), we cannot remove
-  // unreachable blocks in the above loop.  It might delete instructions pointed
-  // to by `calls`.
-  removeUnreachableBlocks(irfunc);
+  // Inlining a callee with no reachable return leaves unreachable blocks
+  // behind.  We can't drop them inside the loop above (that might free call
+  // instructions still queued for inlining), so we clean up here once the loop
+  // is done.  CleanCFG removes the unreachable blocks; CopyPropagation first
+  // collapses the Assigns the inliner introduced.
   CopyPropagation{}.Run(irfunc);
   CleanCFG{}.Run(irfunc);
 }