Revert the series of commits for removing the return value from swift…

…_retain_noresult.

I asked that the patches were split up so I could do post commit review.

This reverts commit r32059.
This reverts commit r32058.
This reverts commit r32056.
This reverts commit r32055.

Swift SVN r32060

include/swift/Runtime/HeapObject.h

@@ -137,6 +137,7 @@ extern "C" void swift_slowDealloc(void *ptr, size_t bytes, size_t alignMask);
 /// Atomically increments the retain count of an object.
 ///
 /// \param object - may be null, in which case this is a no-op
+/// \return its argument value exactly
 ///
 /// POSSIBILITIES: We may end up wanting a bunch of different variants:
 ///  - the general version which correctly handles null values, swift
@@ -146,13 +147,16 @@ extern "C" void swift_slowDealloc(void *ptr, size_t bytes, size_t alignMask);
 ///      - maybe a variant that can assume a non-null object
 /// It may also prove worthwhile to have this use a custom CC
 /// which preserves a larger set of registers.
-extern "C" void swift_retain(HeapObject *object);
-extern "C" void swift_retain_n(HeapObject *object, uint32_t n);
+extern "C" HeapObject *swift_retain(HeapObject *object);
+extern "C" void swift_retain_noresult(HeapObject *object);
 
-static inline void _swift_retain_inlined(HeapObject *object) {
+extern "C" HeapObject *swift_retain_n(HeapObject *object, uint32_t n);
+
+static inline HeapObject *_swift_retain_inlined(HeapObject *object) {
   if (object) {
     object->refCount.increment();
   }
+  return object;
 }
 
 /// Atomically increments the reference count of an object, unless it has
@@ -323,9 +327,7 @@ class SwiftRAII {
       swift_release(object);
   }
 
-  SwiftRAII(const SwiftRAII &other) {
-    swift_retain(*other);
-    object = *other;
+  SwiftRAII(const SwiftRAII &other) : object(swift_retain(*other)) {
     ;
   }
   SwiftRAII(SwiftRAII &&other) : object(*other) {
@@ -334,8 +336,7 @@ class SwiftRAII {
   SwiftRAII &operator=(const SwiftRAII &other) {
     if (object)
       swift_release(object);
-    swift_retain(*other);
-    object = *other;
+    object = swift_retain(*other);
     return *this;
   }
   SwiftRAII &operator=(SwiftRAII &&other) {

include/swift/Runtime/InstrumentsSupport.h

@@ -26,8 +26,8 @@ extern "C" HeapObject *(*_swift_allocObject)(HeapMetadata const *metadata,
 
 extern "C" BoxPair::Return (*_swift_allocBox)(Metadata const *type);
 
-extern "C" void (*_swift_retain)(HeapObject *object);
-extern "C" void (*_swift_retain_n)(HeapObject *object, uint32_t n);
+extern "C" HeapObject *(*_swift_retain)(HeapObject *object);
+extern "C" HeapObject *(*_swift_retain_n)(HeapObject *object, uint32_t n);
 extern "C" HeapObject *(*_swift_tryRetain)(HeapObject *object);
 extern "C" bool (*_swift_isDeallocating)(HeapObject *object);
 extern "C" void (*_swift_release)(HeapObject *object);

lib/IRGen/GenHeap.cpp

@@ -791,7 +791,7 @@ static void emitStoreWeakLikeCall(IRGenFunction &IGF,
   call->setDoesNotThrow();
 }
 
-/// Emit a call to swift_retain.  In general, you should not be using
+/// Emit a call to swift_retain_noresult.  In general, you should not be using
 /// this routine; instead you should use emitRetain, which properly
 /// balances the retain.
 void IRGenFunction::emitRetainCall(llvm::Value *value) {
@@ -800,7 +800,7 @@ void IRGenFunction::emitRetainCall(llvm::Value *value) {
     value = Builder.CreateBitCast(value, IGM.RefCountedPtrTy);
 
   // Emit the call.
-  llvm::CallInst *call = Builder.CreateCall(IGM.getRetainFn(), value);
+  llvm::CallInst *call = Builder.CreateCall(IGM.getRetainNoResultFn(), value);
   call->setCallingConv(IGM.RuntimeCC);
   call->setDoesNotThrow();
 }

lib/IRGen/RuntimeFunctions.def

@@ -113,8 +113,8 @@ FUNCTION(CopyPOD,        swift_copyPOD,       RuntimeCC,
          ARGS(OpaquePtrTy, OpaquePtrTy, TypeMetadataPtrTy),
          ATTRS(NoUnwind))
 
-// void swift_retain(void *ptr);
-FUNCTION(Retain, swift_retain, RuntimeCC,
+// void swift_retain_noresult(void *ptr);
+FUNCTION(RetainNoResult, swift_retain_noresult, RuntimeCC,
          RETURNS(VoidTy),
          ARGS(RefCountedPtrTy),
          ATTRS(NoUnwind))

lib/LLVMPasses/ARCEntryPointBuilder.h

@@ -41,6 +41,7 @@ class ARCEntryPointBuilder {
 
   // The constant cache.
   NullablePtr<Constant> Retain;
+  NullablePtr<Constant> RetainNoResult;
   NullablePtr<Constant> CheckUnowned;
   NullablePtr<Constant> RetainN;
   NullablePtr<Constant> ReleaseN;
@@ -49,7 +50,8 @@ class ARCEntryPointBuilder {
   NullablePtr<Type> ObjectPtrTy;
 
 public:
-  ARCEntryPointBuilder(Function &F) : B(F.begin()), Retain(), ObjectPtrTy() {}
+  ARCEntryPointBuilder(Function &F) : B(F.begin()), Retain(), RetainNoResult(),
+                                      ObjectPtrTy() {}
   ~ARCEntryPointBuilder() = default;
   ARCEntryPointBuilder(ARCEntryPointBuilder &&) = delete;
   ARCEntryPointBuilder(const ARCEntryPointBuilder &) = delete;
@@ -61,6 +63,11 @@ class ARCEntryPointBuilder {
     B.SetInsertPoint(I);
   }
 
+  CallInst *createRetainNoResult(Value *V) {
+    V = B.CreatePointerCast(V, getObjectPtrTy());
+    return B.CreateCall(getRetainNoResult(), V);
+  }
+
   Value *createInsertValue(Value *V1, Value *V2, unsigned Idx) {
     return B.CreateInsertValue(V1, V2, Idx);
   }
@@ -122,12 +129,26 @@ class ARCEntryPointBuilder {
     auto &M = getModule();
     auto AttrList = AttributeSet::get(
         M.getContext(), AttributeSet::FunctionIndex, Attribute::NoUnwind);
-    Retain = M.getOrInsertFunction("swift_retain", AttrList,
-                                   Type::getVoidTy(M.getContext()),
+    Retain = M.getOrInsertFunction("swift_retain", AttrList, ObjectPtrTy,
                                    ObjectPtrTy, nullptr);
     return Retain.get();
   }
 
+  /// getRetainNoResult - Return a callable function for swift_retain_noresult.
+  Constant *getRetainNoResult() {
+    if (RetainNoResult)
+      return RetainNoResult.get();
+    auto *ObjectPtrTy = getObjectPtrTy();
+    auto &M = getModule();
+    auto AttrList = AttributeSet::get(M.getContext(), 1, Attribute::NoCapture);
+    AttrList = AttrList.addAttribute(
+        M.getContext(), AttributeSet::FunctionIndex, Attribute::NoUnwind);
+    RetainNoResult = M.getOrInsertFunction(
+        "swift_retain_noresult", AttrList,
+        Type::getVoidTy(M.getContext()), ObjectPtrTy, nullptr);
+    return RetainNoResult.get();
+  }
+
   Constant *getCheckUnowned() {
     if (CheckUnowned)
       return CheckUnowned.get();
@@ -153,8 +174,7 @@ class ARCEntryPointBuilder {
     auto *Int32Ty = Type::getInt32Ty(M.getContext());
     auto AttrList = AttributeSet::get(
         M.getContext(), AttributeSet::FunctionIndex, Attribute::NoUnwind);
-    RetainN = M.getOrInsertFunction("swift_retain_n", AttrList,
-                                    Type::getVoidTy(M.getContext()),
+    RetainN = M.getOrInsertFunction("swift_retain_n", AttrList, ObjectPtrTy,
                                     ObjectPtrTy, Int32Ty, nullptr);
     return RetainN.get();
   }

lib/LLVMPasses/LLVMARCContract.cpp

@@ -34,6 +34,7 @@ STATISTIC(NumRetainReleasesEliminatedByMergingIntoRetainReleaseN,
 namespace {
 
 struct LocalState {
+  Value *CurrentLocalUpdate;
   TinyPtrVector<CallInst *> RetainList;
   TinyPtrVector<CallInst *> ReleaseList;
 };
@@ -62,13 +63,31 @@ class SwiftARCContractImpl {
 
   /// The entry point builder that is used to construct ARC entry points.
   ARCEntryPointBuilder B;
+
+  /// Since all of the calls are canonicalized, we know that we can just walk
+  /// through the function and collect the interesting heap object definitions
+  /// by getting the argument to these functions.
+  DenseMap<Value *, TinyPtrVector<Instruction *>> DefsOfValue;
+
+  /// Keep track of which order we see values in since iteration over a densemap
+  /// isn't in a deterministic order, and isn't efficient anyway.
+  ///
+  /// TODO: Maybe this should be merged into DefsOfValue in a MapVector?
+  SmallVector<Value *, 16> DefOrder;
+
 public:
   SwiftARCContractImpl(Function &InF) : Changed(false), F(InF), B(F) {}
 
   // The top level run routine of the pass.
   bool run();
 
 private:
+  /// Perform single basic block optimizations.
+  ///
+  /// This means changing retain_no_return into retains, finding return values,
+  /// and merging retains, releases.
+  void performSingleBBOpts();
+
   /// Perform the RRN Optimization given the current state that we are
   /// tracking. This is called at the end of BBs and if we run into an unknown
   /// call.
@@ -88,11 +107,22 @@ performRRNOptimization(DenseMap<Value *, LocalState> &PtrToLocalStateMap) {
     if (RetainList.size() > 1) {
       // Create the retainN call right by the first retain.
       B.setInsertPoint(RetainList[0]);
-      B.createRetainN(RetainList[0]->getArgOperand(0), RetainList.size());
+      auto &CI = *B.createRetainN(RetainList[0]->getArgOperand(0),
+                                  RetainList.size());
+
+      // Change the Local Entry to be the new retainN call.
+      P.second.CurrentLocalUpdate = &CI;
+
+      // Change GlobalEntry to track the new retainN instruction instead of
+      // the last retain that was seen.
+      TinyPtrVector<Instruction *> &GlobalEntry = DefsOfValue[ArgVal];
+      GlobalEntry.pop_back();
+      GlobalEntry.push_back(&CI);
 
       // Replace all uses of the retain instructions with our new retainN and
       // then delete them.
       for (auto *Inst : RetainList) {
+        Inst->replaceAllUsesWith(&CI);
         Inst->eraseFromParent();
         NumRetainReleasesEliminatedByMergingIntoRetainReleaseN++;
       }
@@ -120,10 +150,11 @@ performRRNOptimization(DenseMap<Value *, LocalState> &PtrToLocalStateMap) {
   }
 }
 
+void SwiftARCContractImpl::performSingleBBOpts() {
+  // Do a first pass over the function, collecting all interesting definitions.
 
-bool SwiftARCContractImpl::run() {
-  // Perform single BB optimizations and gather information in prepration for
-  // intra-BB retain/release merging.
+  // In this pass, we rewrite any intra-block uses that we can, since the
+  // SSAUpdater doesn't handle them.
   DenseMap<Value *, LocalState> PtrToLocalStateMap;
   for (BasicBlock &BB : F) {
     for (auto II = BB.begin(), IE = BB.end(); II != IE; ) {
@@ -138,12 +169,41 @@ bool SwiftARCContractImpl::run() {
         Inst.eraseFromParent();
         ++NumNoopDeleted;
         continue;
-      case RT_Retain: {
-        auto *CI = cast<CallInst>(&Inst);
-        auto *ArgVal = CI->getArgOperand(0);
+      case RT_Retain:
+        llvm_unreachable("This should be canonicalized away!");
+      case RT_RetainNoResult: {
+        auto *ArgVal = cast<CallInst>(Inst).getArgOperand(0);
+
+        B.setInsertPoint(&Inst);
+        CallInst &CI = *B.createRetain(ArgVal);
+        Inst.eraseFromParent();
+
+        if (!isa<Instruction>(ArgVal) && !isa<Argument>(ArgVal))
+          continue;
+
+        TinyPtrVector<Instruction *> &GlobalEntry = DefsOfValue[ArgVal];
+
+        // If this is the first definition of a value for the argument that
+        // we've seen, keep track of it in DefOrder.
+        if (GlobalEntry.empty())
+          DefOrder.push_back(ArgVal);
 
         LocalState &LocalEntry = PtrToLocalStateMap[ArgVal];
-        LocalEntry.RetainList.push_back(CI);
+
+        // Check to see if there is already an entry for this basic block.  If
+        // there is another local entry, switch to using the local value and
+        // remove the previous value from the GlobalEntry.
+        if (LocalEntry.CurrentLocalUpdate) {
+          Changed = true;
+          CI.setArgOperand(0, LocalEntry.CurrentLocalUpdate);
+          assert(GlobalEntry.back() == LocalEntry.CurrentLocalUpdate &&
+                 "Local/Global mismatch?");
+          GlobalEntry.pop_back();
+        }
+
+        LocalEntry.CurrentLocalUpdate = &CI;
+        LocalEntry.RetainList.push_back(&CI);
+        GlobalEntry.push_back(&CI);
         continue;
       }
       case RT_Release: {
@@ -170,6 +230,18 @@ bool SwiftARCContractImpl::run() {
         break;
       }
 
+      // Check to see if there are any uses of a value in the LocalUpdates
+      // map.  If so, remap it now to the locally defined version.
+      for (unsigned i = 0, e = Inst.getNumOperands(); i != e; ++i) {
+        auto Iter = PtrToLocalStateMap.find(Inst.getOperand(i));
+        if (Iter != PtrToLocalStateMap.end()) {
+          if (Value *V = Iter->second.CurrentLocalUpdate) {
+            Changed = true;
+            Inst.setOperand(i, V);
+          }
+        }
+      }
+
       if (Kind != RT_Unknown)
         continue;
 
@@ -199,6 +271,64 @@ bool SwiftARCContractImpl::run() {
     performRRNOptimization(PtrToLocalStateMap);
     PtrToLocalStateMap.clear();
   }
+}
+
+bool SwiftARCContractImpl::run() {
+  // Perform single BB optimizations and gather information in prepration for
+  // the multiple BB optimizations.
+  performSingleBBOpts();
+
+  // Now that we've collected all of the interesting heap object values that are
+  // passed into argument-returning functions, rewrite uses of these pointers
+  // with optimized lifetime-shorted versions of it.
+  for (Value *Ptr : DefOrder) {
+    // If Ptr is an instruction, remember its block.  If not, use the entry
+    // block as its block (it must be an argument, constant, etc).
+    BasicBlock *PtrBlock;
+    if (auto *PI = dyn_cast<Instruction>(Ptr))
+      PtrBlock = PI->getParent();
+    else
+      PtrBlock = &F.getEntryBlock();
+
+    TinyPtrVector<Instruction *> &Defs = DefsOfValue[Ptr];
+    // This is the same problem as SSA construction, so we just use LLVM's
+    // SSAUpdater, with each retain as a definition of the virtual value.
+    SSAUpdater Updater;
+    Updater.Initialize(Ptr->getType(), Ptr->getName());
+
+    // Set the return value of each of these calls as a definition of the
+    // virtual value.
+    for (auto *D : Defs)
+      Updater.AddAvailableValue(D->getParent(), D);
+
+    // If we didn't add a definition for Ptr's block, then Ptr itself is
+    // available in its block.
+    if (!Updater.HasValueForBlock(PtrBlock))
+      Updater.AddAvailableValue(PtrBlock, Ptr);
+
+    // Rewrite uses of Ptr to their optimized forms.
+    //
+    // NOTE: We are assuming that our Ptrs are not constants meaning that we
+    // know that users can not be constant expressions.
+    for (auto UI = Ptr->user_begin(), E = Ptr->user_end(); UI != E; ) {
+      // Make sure to increment the use iterator before potentially rewriting
+      // it.
+      Use &U = UI.getUse();
+      ++UI;
+
+      // If the use is in the same block that defines it and the User is not a
+      // PHI node, then this is a local use that shouldn't be rewritten.
+      auto *User = cast<Instruction>(U.getUser());
+      if (User->getParent() == PtrBlock && !isa<PHINode>(User))
+        continue;
+
+      // Otherwise, change it if profitable!
+      Updater.RewriteUse(U);
+
+      if (U.get() != Ptr)
+        Changed = true;
+    }
+  }
 
   return Changed;
 }