[ObjC][ARC] Handle operand bundle "clang.arc.attachedcall" on targets that don't use the inline asm marker

[ahatanak]

Summary

https://reviews.llvm.org/D111331 makes clang use operand bundle "clang.arc.attachedcall" on x86-64, which, unlike arm, doesn't use the inline asm marker for the retainRV/autoreleaseRV handshake. This patch makes the changes to the ARC middle-end passes that are needed to handle the operand bundle on targets that don't use the marker.

Note that anyone who wants to use the operand bundle on their target has to teach their backend to handle the operand bundle. The x86-64 backend already knows about the operand bundle (see https://reviews.llvm.org/D94597).

[rjmccall]

Okay, so does this mean that this works on arbitrary targets now? If so, should Clang be using it unconditionally?

[ahatanak]

No, only arm64 and x86-64 can correctly handle the operand bundle in the backend. I should mention that this only changes the middle-end passes and we still need to make changes to the backend to handle arbitrary targets.

[rjmccall]

Ok. I'm not an expert in this code, but it seems fine to me.

[fhahn]

LGTM, thanks! I left 2 small suggestions that can directly be addressed before committing.

llvm/lib/Transforms/ObjCARC/ObjCARC.h

159

It might be worth adding a quick comment here, to explain what UseMarker indicates here.

llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp

437–442

nit: When I first read the code without looking at the precise definition I thought this is a bundle instruction, rather than whether the instruction is contained in a bundle. Perhaps the name could be improved slightly, e.g. IsInstContainedInBundle

llvm/lib/Transforms/ObjCARC/ObjCARC.h

 /// going to be removed from the IR before WinEHPrepare.
 CallInst *createCallInstWithColors(
     FunctionCallee Func, ArrayRef<Value *> Args, const Twine &NameStr,
     Instruction *InsertBefore,
     const DenseMap<BasicBlock *, ColorVector> &BlockColors);
 
 class BundledRetainClaimRVs {
 public:
-  BundledRetainClaimRVs(bool ContractPass) : ContractPass(ContractPass) {}
+  BundledRetainClaimRVs(bool ContractPass, bool UseMarker)
+      : ContractPass(ContractPass), UseMarker(UseMarker) {}
   ~BundledRetainClaimRVs();
 
   /// Insert a retainRV/claimRV call to the normal destination blocks of invokes
   /// with operand bundle "clang.arc.attachedcall". If the edge to the normal
   /// destination block is a critical edge, split it.
   std::pair<bool, bool> insertAfterInvokes(Function &F, DominatorTree *DT);
 
   /// Insert a retainRV/claimRV call.
void eraseInst(CallInst *CI) {
     EraseInstruction(CI);
   }
 
 private:
   /// A map of inserted retainRV/claimRV calls to annotated calls/invokes.
   DenseMap<CallInst *, CallBase *> RVCalls;
 
   bool ContractPass;
+
+  /// Indicates whether the target uses a special inline-asm marker.
+  bool UseMarker;
 };
 
 } // end namespace objcarc
 } // end namespace llvm
 
 #endif

llvm/lib/Transforms/ObjCARC/ObjCARC.cpp

if (ContractPass) {
       CallBase *CB = P.second;
       // At this point, we know that the annotated calls can't be tail calls
       // as they are followed by marker instructions and retainRV/claimRV
       // calls. Mark them as notail so that the backend knows these calls
       // can't be tail calls.
       if (auto *CI = dyn_cast<CallInst>(CB))
         CI->setTailCallKind(CallInst::TCK_NoTail);
 
-      // Remove the ARC intrinsic function operand from the operand bundle.
+      if (UseMarker) {
+        // Remove the retainRV/claimRV function operand from the operand bundle
+        // to reflect the fact that the backend is responsible for emitting only
+        // the marker instruction, but not the retainRV/claimRV call.
         OperandBundleDef OB("clang.arc.attachedcall", None);
         auto *NewCB = CallBase::Create(CB, OB, CB);
         CB->replaceAllUsesWith(NewCB);
         CB->eraseFromParent();
-    } else {
-      EraseInstruction(P.first);
       }
     }
 
+    if (!ContractPass || !UseMarker)
+      EraseInstruction(P.first);
+  }
+
   RVCalls.clear();
 }

llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp

case ARCInstKind::Retain:
     // Attempt to convert retains to retainrvs if they are next to function
     // calls.
     if (!optimizeRetainCall(F, Inst))
       return false;
     // If we succeed in our optimization, fall through.
     LLVM_FALLTHROUGH;
   case ARCInstKind::RetainRV:
   case ARCInstKind::ClaimRV: {
-    // If we're compiling for a target which needs a special inline-asm
-    // marker to do the return value optimization and the retainRV/claimRV call
-    // wasn't bundled with a call, insert the marker now.
+    bool IsInstContainedInBundle = BundledInsts->contains(Inst);
+
+    // Return now if the target doesn't need a special inline-asm marker. Return
+    // true if this is a bundled retainRV/claimRV call, which is going to be
+    // erased at the end of this pass, to avoid undoing objc-arc-expand and
+    // replacing uses of the retainRV/claimRV call's argument with its result.
     if (!RVInstMarker)
-      return false;
+      return IsInstContainedInBundle;
+
+    // The target needs a special inline-asm marker.
 
-    if (BundledInsts->contains(Inst))
+    // We don't have to emit the marker if this is a bundled call since the
+    // backend is responsible for emitting it. Return false to undo
+    // objc-arc-expand.
+    if (IsInstContainedInBundle)
       return false;
 
     BasicBlock::iterator BBI = Inst->getIterator();
     BasicBlock *InstParent = Inst->getParent();
 
     // Step up to see if the call immediately precedes the RV call.
     // If it's an invoke, we have to cross a block boundary. And we have
     // to carefully dodge no-op instructions.

 bool ObjCARCContract::run(Function &F, AAResults *A, DominatorTree *D) {
   if (!EnableARCOpts)
     return false;
 
   Changed = CFGChanged = false;
   AA = A;
   DT = D;
   PA.setAA(A);
-  BundledRetainClaimRVs BRV(true);
+  BundledRetainClaimRVs BRV(true, RVInstMarker);
   BundledInsts = &BRV;
 
   std::pair<bool, bool> R = BundledInsts->insertAfterInvokes(F, DT);
   Changed |= R.first;
   CFGChanged |= R.second;
 
   DenseMap<BasicBlock *, ColorVector> BlockColors;
   if (F.hasPersonalityFn() &&

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

void ObjCARCOpt::init(Module &M) {
   EP.init(&M);
 }
 
 bool ObjCARCOpt::run(Function &F, AAResults &AA) {
   if (!EnableARCOpts)
     return false;
 
   Changed = CFGChanged = false;
-  BundledRetainClaimRVs BRV(false);
+  BundledRetainClaimRVs BRV(false, objcarc::getRVInstMarker(*F.getParent()));
   BundledInsts = &BRV;
 
   LLVM_DEBUG(dbgs() << "<<< ObjCARCOpt: Visiting Function: " << F.getName()
                     << " >>>"
                        "\n");
 
   std::pair<bool, bool> R = BundledInsts->insertAfterInvokes(F, nullptr);
   Changed |= R.first;

llvm/test/Transforms/ObjCARC/contract-attached-call-no-marker.ll

+; RUN: opt -objc-arc-contract -S < %s | FileCheck %s
+; RUN: opt -passes=objc-arc-contract -S < %s | FileCheck %s
+
+; CHECK-LABEL: define void @test0() {
+; CHECK: %[[CALL:.*]] = notail call i8* @foo() [ "clang.arc.attachedcall"(i8* (i8*)* @llvm.objc.retainAutoreleasedReturnValue) ]
+; CHECK-NEXT: ret void
+
+define void @test0() {
+  %call1 = call i8* @foo() [ "clang.arc.attachedcall"(i8* (i8*)* @llvm.objc.retainAutoreleasedReturnValue) ]
+  ret void
+}
+
+; CHECK-LABEL: define void @test1() {
+; CHECK: %[[CALL:.*]] = notail call i8* @foo() [ "clang.arc.attachedcall"(i8* (i8*)* @llvm.objc.unsafeClaimAutoreleasedReturnValue) ]
+; CHECK-NEXT: ret void
+
+define void @test1() {
+  %call1 = call i8* @foo() [ "clang.arc.attachedcall"(i8* (i8*)* @llvm.objc.unsafeClaimAutoreleasedReturnValue) ]
+  ret void
+}
+
+declare i8* @foo()
+declare i8* @llvm.objc.retainAutoreleasedReturnValue(i8*)
+declare i8* @llvm.objc.unsafeClaimAutoreleasedReturnValue(i8*)