[SIL Optimization] Improve dead-code in OSLogOptimization pass, fix a bug and add helper functions to the new InstructionDeleter utility

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -79,6 +79,13 @@ class InstructionDeleter {
   /// up.
   void trackIfDead(SILInstruction *inst);
 
+  /// If the instruction \p inst is dead, delete it immediately and record
+  /// its operands so that they can be cleaned up later.
+  void deleteIfDead(
+      SILInstruction *inst,
+      llvm::function_ref<void(SILInstruction *)> callback =
+          [](SILInstruction *) {});
+
   /// Delete the instruction \p inst and record instructions that may become
   /// dead because of the removal of \c inst. This function will add necessary
   /// ownership instructions to fix the lifetimes of the operands of \c inst to
@@ -130,6 +137,14 @@ class InstructionDeleter {
   void
   cleanUpDeadInstructions(llvm::function_ref<void(SILInstruction *)> callback =
                               [](SILInstruction *) {});
+
+  /// Recursively visit users of \c inst  (including \c inst)and delete
+  /// instructions that are dead (including \c inst). Invoke the \c callback on
+  /// instructions that are deleted.
+  void recursivelyDeleteUsersIfDead(
+      SILInstruction *inst,
+      llvm::function_ref<void(SILInstruction *)> callback =
+                                    [](SILInstruction *) {});
 };
 
 /// If \c inst is dead, delete it and recursively eliminate all code that
@@ -147,6 +162,9 @@ void eliminateDeadInstruction(
     SILInstruction *inst, llvm::function_ref<void(SILInstruction *)> callback =
                               [](SILInstruction *) {});
 
+/// Return the number of @inout arguments passed to the given apply site.
+unsigned getNumInOutArguments(FullApplySite applySite);
+
 /// For each of the given instructions, if they are dead delete them
 /// along with their dead operands. Note this utility must be phased out and
 /// replaced by \c eliminateDeadInstruction  and

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -1059,31 +1059,152 @@ static bool checkOSLogMessageIsConstant(SingleValueInstruction *osLogMessage,
   return errorDetected;
 }
 
+using CallbackTy = llvm::function_ref<void(SILInstruction *)>;
+
+/// Return true iff the given address-valued instruction has only stores into
+/// it. This function tests for the conditions under which a call, that was
+/// constant evaluated, that writes into the address-valued instruction can be
+/// considered as a point store and exploits it to remove such uses.
+/// TODO: eventually some of this logic can be moved to
+/// PredictableDeadAllocElimination pass, but the assumption about constant
+/// evaluable functions taking inout parameters is not easily generalizable to
+/// arbitrary non-constant contexts where the function could be used. The logic
+/// here is relying on the fact that the constant_evaluable function has been
+/// evaluated and therefore doesn't have any side-effects.
+static bool hasOnlyStoreUses(SingleValueInstruction *addressInst) {
+  for (Operand *use : addressInst->getUses()) {
+    SILInstruction *user = use->getUser();
+    switch (user->getKind()) {
+    default:
+      return false;
+    case SILInstructionKind::BeginAccessInst: {
+      if (!hasOnlyStoreUses(cast<BeginAccessInst>(user)))
+        return false;
+      continue;
+    }
+    case SILInstructionKind::StoreInst: {
+      // For now, ignore assigns as we need to destroy_addr its dest if it
+      // is deleted.
+      if (cast<StoreInst>(user)->getOwnershipQualifier() ==
+          StoreOwnershipQualifier::Assign)
+        return false;
+      continue;
+    }
+    case SILInstructionKind::EndAccessInst:
+    case SILInstructionKind::DestroyAddrInst:
+    case SILInstructionKind::InjectEnumAddrInst:
+    case SILInstructionKind::DeallocStackInst:
+      continue;
+    case SILInstructionKind::ApplyInst: {
+      ApplyInst *apply = cast<ApplyInst>(user);
+      SILFunction *callee = apply->getCalleeFunction();
+      if (!callee || !isConstantEvaluable(callee) || !apply->use_empty())
+        return false;
+      // Note that since we are looking at an alloc_stack used to produce the
+      // OSLogMessage instance, this constant_evaluable call should have been
+      // evaluated successfully by the evaluator. Otherwise, we would have
+      // reported an error earlier. Therefore, all values manipulated by such
+      // a call are symbolic constants and the call would not have any global
+      // side effects. The following logic relies on this property.
+      // If there are other indirect writable results for the call other than
+      // the alloc_stack we are checking, it may not be dead. Therefore, bail
+      // out.
+      FullApplySite applySite(apply);
+      unsigned numWritableArguments =
+        getNumInOutArguments(applySite) + applySite.getNumIndirectSILResults();
+      if (numWritableArguments > 1)
+        return false;
+      SILArgumentConvention convention = applySite.getArgumentConvention(*use);
+      if (convention == SILArgumentConvention::Indirect_In_Guaranteed ||
+          convention == SILArgumentConvention::Indirect_In_Constant ||
+          convention == SILArgumentConvention::Indirect_In_Guaranteed) {
+        if (numWritableArguments > 0)
+          return false;
+      }
+      // Here, either there are no writable parameters or the alloc_stack
+      // is the only writable parameter.
+      continue;
+    }
+    }
+  }
+  return true;
+}
+
+/// Delete the given alloc_stack instruction by deleting the users of the
+/// instruction. In case the user is a begin_apply, recursively delete the users
+/// of begin_apply. This will also fix the lifetimes of the deleted instructions
+/// whenever possible.
+static void forceDeleteAllocStack(SingleValueInstruction *inst,
+                                  InstructionDeleter &deleter,
+                                  CallbackTy callback) {
+  SmallVector<SILInstruction *, 8> users;
+  for (Operand *use : inst->getUses())
+    users.push_back(use->getUser());
+
+  for (SILInstruction *user : users) {
+    if (isIncidentalUse(user))
+      continue;
+    if (isa<DestroyAddrInst>(user)) {
+      deleter.forceDelete(user, callback);
+      continue;
+    }
+    if (isa<BeginAccessInst>(user)) {
+      forceDeleteAllocStack(cast<BeginAccessInst>(user), deleter, callback);
+      continue;
+    }
+    deleter.forceDeleteAndFixLifetimes(user, callback);
+  }
+  deleter.forceDelete(inst, callback);
+}
+
+/// Delete \c inst , if it is dead, along with its dead users and invoke the
+/// callback whever an instruction is deleted.
+static void deleteInstructionWithUsersAndFixLifetimes(
+    SILInstruction *inst, InstructionDeleter &deleter, CallbackTy callback) {
+  // If this is an alloc_stack, it can be eliminated as long as it is only
+  // stored into or destroyed.
+  if (AllocStackInst *allocStack = dyn_cast<AllocStackInst>(inst)) {
+    if (hasOnlyStoreUses(allocStack))
+      forceDeleteAllocStack(allocStack, deleter, callback);
+    return;
+  }
+  deleter.recursivelyDeleteUsersIfDead(inst, callback);
+}
+
 /// Try to dead-code eliminate the OSLogMessage instance \c oslogMessage passed
 /// to the os log call and clean up its dependencies. If the instance cannot be
 /// eliminated, it implies that either the instance is not auto-generated or the
 /// implementation of the os log overlay is incorrect. Therefore emit
 /// diagnostics in such cases.
 static void tryEliminateOSLogMessage(SingleValueInstruction *oslogMessage) {
-  // Collect the set of root instructions that could be dead due to constant
-  // folding. These include the oslogMessage initialzer call and its transitive
-  // users.
-  SmallVector<SILInstruction *, 8> oslogMessageUsers;
-  getTransitiveUsers(oslogMessage, oslogMessageUsers);
-
   InstructionDeleter deleter;
-  for (SILInstruction *user : oslogMessageUsers)
-    deleter.trackIfDead(user);
-  deleter.trackIfDead(oslogMessage);
-
-  bool isOSLogMessageDead = false;
-  deleter.cleanUpDeadInstructions([&](SILInstruction *deadInst) {
-    if (deadInst == oslogMessage)
-      isOSLogMessageDead = true;
-  });
-  // At this point, the OSLogMessage instance must be deleted if
-  // the overlay implementation (or its extensions by users) is correct.
-  if (!isOSLogMessageDead) {
+  // List of instructions that are possibly dead.
+  SmallVector<SILInstruction *, 4> worklist = {oslogMessage};
+  // Set of all deleted instructions.
+  SmallPtrSet<SILInstruction *, 4> deletedInstructions;
+  unsigned startIndex = 0;
+  while (startIndex < worklist.size()) {
+    SILInstruction *inst = worklist[startIndex++];
+    if (deletedInstructions.count(inst))
+      continue;
+    deleteInstructionWithUsersAndFixLifetimes(
+        inst, deleter, [&](SILInstruction *deadInst) {
+          // Add operands of all deleted instructions to the worklist so that
+          // they can be recursively deleted if possible.
+          for (Operand &operand : deadInst->getAllOperands()) {
+            if (SILInstruction *definingInstruction =
+                    operand.get()->getDefiningInstruction()) {
+              if (!deletedInstructions.count(definingInstruction))
+                worklist.push_back(definingInstruction);
+            }
+          }
+          (void)deletedInstructions.insert(deadInst);
+        });
+  }
+  deleter.cleanUpDeadInstructions();
+  // If the OSLogMessage instance is not deleted, the overlay implementation
+  // (or its extensions by users) is incorrect.
+  if (!deletedInstructions.count(oslogMessage)) {
     SILFunction *fun = oslogMessage->getFunction();
     diagnose(fun->getASTContext(), oslogMessage->getLoc().getSourceLoc(),
              diag::oslog_message_alive_after_opts);

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -189,21 +189,53 @@ static bool hasOnlyEndOfScopeOrDestroyUses(SILInstruction *inst) {
   return true;
 }
 
-/// Return true iff the \p applySite calls a constant evaluable function and if
-/// it is read-only which implies the following:
-///   (1) The call does not write into any memory location.
+unsigned swift::getNumInOutArguments(FullApplySite applySite) {
+  assert(applySite);
+  auto substConv = applySite.getSubstCalleeConv();
+  unsigned numIndirectResults = substConv.getNumIndirectSILResults();
+  unsigned numInOutArguments = 0;
+  for (unsigned argIndex = 0; argIndex < applySite.getNumArguments();
+       argIndex++) {
+    // Skip indirect results.
+    if (argIndex < numIndirectResults) {
+      continue;
+    }
+    auto paramNumber = argIndex - numIndirectResults;
+    auto ParamConvention =
+        substConv.getParameters()[paramNumber].getConvention();
+    switch (ParamConvention) {
+    case ParameterConvention::Indirect_Inout:
+    case ParameterConvention::Indirect_InoutAliasable: {
+      numInOutArguments++;
+      break;
+    default:
+      break;
+    }
+    }
+  }
+  return numInOutArguments;
+}
+
+/// Return true iff the \p applySite calls a constant-evaluable function and
+/// it is non-generic and read/destroy only, which means that the call can do
+/// only the following and nothing else:
+///   (1) The call may read any memory location.
 ///   (2) The call may destroy owned parameters i.e., consume them.
-///   (3) The call does not throw or exit the program.
-static bool isReadOnlyConstantEvaluableCall(FullApplySite applySite) {
+///   (3) The call may write into memory locations newly created by the call.
+///   (4) The call may use assertions, which traps at runtime on failure.
+///   (5) The call may return a non-generic value.
+/// Essentially, these are calls whose "effect" is visible only in their return
+/// value or through the parameters that are destroyed. The return value
+/// is also guaranteed to have value semantics as it is non-generic and
+/// reference semantics is not constant evaluable.
+static bool isNonGenericReadOnlyConstantEvaluableCall(FullApplySite applySite) {
   assert(applySite);
   SILFunction *callee = applySite.getCalleeFunction();
   if (!callee || !isConstantEvaluable(callee)) {
     return false;
   }
-  // Here all effects of the call is restricted to its indirect results, which
-  // must have value semantics. If there are no indirect results, the call must
-  // be read-only, except for consuming its operands.
-  return applySite.getNumIndirectSILResults() == 0;
+  return !applySite.hasSubstitutions() && !getNumInOutArguments(applySite) &&
+         !applySite.getNumIndirectSILResults();
 }
 
 /// A scope-affecting instruction is an instruction which may end the scope of
@@ -270,23 +302,25 @@ static bool isScopeAffectingInstructionDead(SILInstruction *inst) {
     return true;
   }
   case SILInstructionKind::ApplyInst: {
-    // The following property holds for constant evaluable functions:
+    // The following property holds for constant-evaluable functions that do
+    // not take arguments of generic type:
     // 1. they do not create objects having deinitializers with global
+    // side effects, as they can only create objects consisting of trivial
+    // values, (non-generic) arrays and strings.
+    // 2. they do not use global variables or call arbitrary functions with
     // side effects.
-    // 2. they do not use global variables and will only use objects reachable
-    // from parameters.
     // The above two properties imply that a value returned by a constant
-    // evaluable function either does not have a deinitializer with global side
-    // effects, or if it does, the deinitializer that has the global side effect
-    // must be that of a parameter.
+    // evaluable function does not have a deinitializer with global side
+    // effects. Therefore, the deinitializer can be sinked.
     //
-    // A read-only constant evaluable call only reads and/or destroys its
-    // parameters. Therefore, if its return value is used only in destroys, the
-    // constant evaluable call can be removed provided the parameters it
-    // consumes are explicitly destroyed at the call site, which is taken care
-    // of by the function: \c deleteInstruction
+    // A generic, read-only constant evaluable call only reads and/or
+    // destroys its (non-generic) parameters. It therefore cannot have any
+    // side effects (note that parameters being non-generic have value
+    // semantics). Therefore, the constant evaluable call can be removed
+    // provided the parameter lifetimes are handled correctly, which is taken
+    // care of by the function: \c deleteInstruction.
     FullApplySite applySite(cast<ApplyInst>(inst));
-    return isReadOnlyConstantEvaluableCall(applySite);
+    return isNonGenericReadOnlyConstantEvaluableCall(applySite);
   }
   default: {
     return false;
@@ -318,9 +352,14 @@ static void destroyConsumedOperandOfDeadInst(Operand &operand) {
   SILValue operandValue = operand.get();
   if (operandValue->getType().isTrivial(*fun))
     return;
+  // Ignore type-dependent operands which are not real operands but are just
+  // there to create use-def dependencies.
+  if (deadInst->isTypeDependentOperand(operand))
+    return;
   // A scope ending instruction cannot be deleted in isolation without removing
   // the instruction defining its operand as well.
   assert(!isEndOfScopeMarker(deadInst) && !isa<DestroyValueInst>(deadInst) &&
+         !isa<DestroyAddrInst>(deadInst) &&
          "lifetime ending instruction is deleted without its operand");
   ValueOwnershipKind operandOwnershipKind = operandValue.getOwnershipKind();
   UseLifetimeConstraint lifetimeConstraint =
@@ -379,7 +418,10 @@ void InstructionDeleter::deleteInstruction(SILInstruction *inst,
   // First drop all references from all instructions to be deleted and then
   // erase the instruction. Note that this is done in this order so that when an
   // instruction is deleted, its uses would have dropped their references.
+  // Note that the toDeleteInsts must also be removed from the tracked
+  // deadInstructions.
   for (SILInstruction *inst : toDeleteInsts) {
+    deadInstructions.remove(inst);
     inst->dropAllReferences();
   }
   for (SILInstruction *inst : toDeleteInsts) {
@@ -428,6 +470,14 @@ static bool hasOnlyIncidentalUses(SILInstruction *inst,
   return true;
 }
 
+void InstructionDeleter::deleteIfDead(SILInstruction *inst,
+                                      CallbackTy callback) {
+  if (isInstructionTriviallyDead(inst) ||
+      isScopeAffectingInstructionDead(inst)) {
+    deleteInstruction(inst, callback, /*Fix lifetime of operands*/ true);
+  }
+}
+
 void InstructionDeleter::forceDeleteAndFixLifetimes(SILInstruction *inst,
                                                     CallbackTy callback) {
   SILFunction *fun = inst->getFunction();
@@ -447,6 +497,18 @@ void InstructionDeleter::forceDelete(SILInstruction *inst,
   deleteInstruction(inst, callback, /*Fix lifetime of operands*/ false);
 }
 
+void InstructionDeleter::recursivelyDeleteUsersIfDead(SILInstruction *inst,
+                                                      CallbackTy callback) {
+  SmallVector<SILInstruction *, 8> users;
+  for (SILValue result : inst->getResults())
+    for (Operand *use : result->getUses())
+      users.push_back(use->getUser());
+
+  for (SILInstruction *user : users)
+    recursivelyDeleteUsersIfDead(user, callback);
+  deleteIfDead(inst, callback);
+}
+
 void swift::eliminateDeadInstruction(SILInstruction *inst,
                                      CallbackTy callback) {
   InstructionDeleter deleter;