[Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.

[jroelofs]

Summary

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-runtime-objc-retain

rdar://79869679

[jroelofs]

clang-format

[jroelofs]

ping

[rjmccall]

IIUC, we sometimes turn -retain message sends into calls to objc_retain and so on. Outside of ARC (the only time they're valid), these calls don't have the semantics of guaranteeing to return their self argument. So I think this change is only fine if we're not applying it when doing that non-ARC transformation.

[jroelofs]

Apply the attribute only to call sites emitted via calls to the intrinsics.

[jroelofs]

Explain why the attribute goes on the call site and not the callee.

[jroelofs]

Add a note/check to the [x retain] => objc_retain(x) test explaining why it shouldn't be lowered as an intrinsic call in the future.

[jroelofs]

In D105671#2884483, @rjmccall wrote:

IIUC, we sometimes turn -retain message sends into calls to objc_retain and so on. Outside of ARC (the only time they're valid), these calls don't have the semantics of guaranteeing to return their self argument. So I think this change is only fine if we're not applying it when doing that non-ARC transformation.

I believe this is covered now. The -retain => objc_retain transformation emits normal function calls, and objc_retain calls are only transformed into intrinsic calls when ARC is enabled.

[rjmccall]

Okay, thanks, seems like a good approach.

clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m

34

This only actually tests for the absence of these lines between the CHECK-LABEL and the first CALLS line below, so I don't think it does much.

[jroelofs]

Rebased.

Also, turns out that stripPointerCasts() can see through the thisreturn attribute, which defeats a self retain optimization, breaking one of the clang tests. I tweaked a callback to make it configurable. Let me know if you see a better way of dealing with that.

[rjmccall]

How does stripPointerCasts "see through" an attribute on the underlying call instruction?

[jroelofs]

In D105671#2978248, @rjmccall wrote:

How does stripPointerCasts "see through" an attribute on the underlying call instruction?

https://github.com/llvm/llvm-project/blob/bf77b11277411f6725cf09a66feb36d2c14bc8a7/llvm/lib/IR/Value.cpp#L652

[rjmccall]

Whoa. That does not seem like it's in the contract for stripPointerCasts() generically. At best that should be enabled by the strip kind.

[nikic]

Please separate the change to stripPointerCasts() into a separate review.

[jroelofs]

In D105671#4547848, @nikic wrote:

Please separate the change to stripPointerCasts() into a separate review.

https://reviews.llvm.org/D156735

[jroelofs]

Move the stripPointerCasts() change into its own review: https://reviews.llvm.org/D156735

[jroelofs]

oops, uploaded the wrong patch to this review.

[jroelofs]

Rebase. Avoid the dependency on D156735

clang/lib/CodeGen/CGCall.cpp

static llvm::Value *tryRemoveRetainOfSelf(CodeGenFunction &CGF,
                                           llvm::Value *result) {
   // This is only applicable to a method with an immutable 'self'.
   const ObjCMethodDecl *method =
     dyn_cast_or_null<ObjCMethodDecl>(CGF.CurCodeDecl);
   if (!method) return nullptr;
   const VarDecl *self = method->getSelfDecl();
   if (!self->getType().isConstQualified()) return nullptr;
 
-  // Look for a retain call.
-  llvm::CallInst *retainCall =
-    dyn_cast<llvm::CallInst>(result->stripPointerCasts());
+  // Look for a retain call. Note: stripPointerCasts looks through returned arg
+  // functions, which would cause us to miss the retain.
+  llvm::CallInst *retainCall = dyn_cast<llvm::CallInst>(result);
   if (!retainCall || retainCall->getCalledOperand() !=
                          CGF.CGM.getObjCEntrypoints().objc_retain)
     return nullptr;
 
   // Look for an ordinary load of 'self'.
   llvm::Value *retainedValue = retainCall->getArgOperand(0);
   llvm::LoadInst *load =
     dyn_cast<llvm::LoadInst>(retainedValue->stripPointerCasts());

clang/test/CodeGenObjC/arc.m

 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -Wno-objc-root-class -Wno-incompatible-pointer-types -Wno-arc-unsafe-retained-assign -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -O2 -disable-llvm-passes -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -Wno-objc-root-class -Wno-incompatible-pointer-types -Wno-arc-unsafe-retained-assign -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -o - %s | FileCheck -check-prefix=CHECK-GLOBALS %s
 
 // rdar://13129783. Check both native/non-native arc platforms. Here we check
 // that they treat nonlazybind differently.
 // RUN: %clang_cc1 -fobjc-runtime=macosx-10.6.0 -triple x86_64-apple-darwin10 -Wno-objc-root-class -Wno-incompatible-pointer-types -Wno-arc-unsafe-retained-assign -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -o - %s | FileCheck -check-prefix=ARC-ALIEN %s
 // RUN: %clang_cc1 -fobjc-runtime=macosx-10.7.0 -triple x86_64-apple-darwin11 -Wno-objc-root-class -Wno-incompatible-pointer-types -Wno-arc-unsafe-retained-assign -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -o - %s | FileCheck -check-prefix=ARC-NATIVE %s
 
 // ARC-ALIEN: declare extern_weak void @llvm.objc.storeStrong(ptr, ptr)
-// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retain(ptr)
-// ARC-ALIEN: declare extern_weak ptr @llvm.objc.autoreleaseReturnValue(ptr)
+// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retain(ptr returned)
+// ARC-ALIEN: declare extern_weak ptr @llvm.objc.autoreleaseReturnValue(ptr returned)
 // ARC-ALIEN: declare ptr @objc_msgSend(ptr, ptr, ...) [[NLB:#[0-9]+]]
 // ARC-ALIEN: declare extern_weak void @llvm.objc.release(ptr)
-// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retainAutoreleasedReturnValue(ptr)
+// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retainAutoreleasedReturnValue(ptr returned)
 // ARC-ALIEN: declare extern_weak ptr @llvm.objc.initWeak(ptr, ptr)
 // ARC-ALIEN: declare extern_weak ptr @llvm.objc.storeWeak(ptr, ptr)
 // ARC-ALIEN: declare extern_weak ptr @llvm.objc.loadWeakRetained(ptr)
 // ARC-ALIEN: declare extern_weak void @llvm.objc.destroyWeak(ptr)
-// ARC-ALIEN: declare extern_weak ptr @llvm.objc.autorelease(ptr)
-// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retainAutorelease(ptr)
+// ARC-ALIEN: declare extern_weak ptr @llvm.objc.autorelease(ptr returned)
+// ARC-ALIEN: declare extern_weak ptr @llvm.objc.retainAutorelease(ptr returned)
 
 // ARC-NATIVE: declare void @llvm.objc.storeStrong(ptr, ptr)
-// ARC-NATIVE: declare ptr @llvm.objc.retain(ptr)
-// ARC-NATIVE: declare ptr @llvm.objc.autoreleaseReturnValue(ptr)
+// ARC-NATIVE: declare ptr @llvm.objc.retain(ptr returned)
+// ARC-NATIVE: declare ptr @llvm.objc.autoreleaseReturnValue(ptr returned)
 // ARC-NATIVE: declare ptr @objc_msgSend(ptr, ptr, ...) [[NLB:#[0-9]+]]
 // ARC-NATIVE: declare void @llvm.objc.release(ptr)
-// ARC-NATIVE: declare ptr @llvm.objc.retainAutoreleasedReturnValue(ptr)
+// ARC-NATIVE: declare ptr @llvm.objc.retainAutoreleasedReturnValue(ptr returned)
 // ARC-NATIVE: declare ptr @llvm.objc.initWeak(ptr, ptr)
 // ARC-NATIVE: declare ptr @llvm.objc.storeWeak(ptr, ptr)
 // ARC-NATIVE: declare ptr @llvm.objc.loadWeakRetained(ptr)
 // ARC-NATIVE: declare void @llvm.objc.destroyWeak(ptr)
-// ARC-NATIVE: declare ptr @llvm.objc.autorelease(ptr)
-// ARC-NATIVE: declare ptr @llvm.objc.retainAutorelease(ptr)
+// ARC-NATIVE: declare ptr @llvm.objc.autorelease(ptr returned)
+// ARC-NATIVE: declare ptr @llvm.objc.retainAutorelease(ptr returned)
 
 // CHECK-LABEL: define{{.*}} void @test0
 void test0(id x) {
   // CHECK:      [[X:%.*]] = alloca ptr
   // CHECK-NEXT: [[PARM:%.*]] = call ptr @llvm.objc.retain(ptr {{.*}})
   // CHECK-NEXT: store ptr [[PARM]], ptr [[X]]
   // CHECK-NEXT: [[TMP:%.*]] = load ptr, ptr [[X]]
   // CHECK-NEXT: call void @llvm.objc.release(ptr [[TMP]])

clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m

 void test1(id x) {
   // MSGS: {{call.*@objc_msgSend}}
   // MSGS: {{call.*@objc_msgSend}}
   // MSGS: {{call.*@objc_msgSend}}
   // MSGS: {{call.*@objc_msgSend}}
   // MSGS: {{call.*@objc_msgSend}}
   // CALLS: {{call.*@objc_alloc}}
   // CALLS: {{call.*@objc_allocWithZone}}
+
+  // Note that calls to the intrinsics are not allowed for
+  // retain/release/autorelease they're marked `thisreturn`, which isn't
+  // guaranteed to be true for classes that define their own `-retain`, for
+  // example. Be sure to keep these as normal function calls:
   // CALLS: {{call.*@objc_retain}}
   // CALLS: {{call.*@objc_release}}
   // CALLS: {{tail call.*@objc_autorelease}}
   [NSObject alloc];
   [NSObject allocWithZone:nil];
   [x retain];
   [x release];
   [x autorelease];

llvm/include/llvm/IR/Intrinsics.td

def int_gcwrite : Intrinsic<[],
                              NoCapture<ArgIndex<2>>]>;
 
 //===------------------- ObjC ARC runtime Intrinsics --------------------===//
 //
 // Note these are to support the Objective-C ARC optimizer which wants to
 // eliminate retain and releases where possible.
 
 def int_objc_autorelease                    : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_autoreleasePoolPop             : Intrinsic<[], [llvm_ptr_ty]>;
 def int_objc_autoreleasePoolPush            : Intrinsic<[llvm_ptr_ty], []>;
 def int_objc_autoreleaseReturnValue         : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_copyWeak                       : Intrinsic<[],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_destroyWeak                    : Intrinsic<[], [llvm_ptr_ty]>;
 def int_objc_initWeak                       : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_loadWeak                       : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_loadWeakRetained               : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_moveWeak                       : Intrinsic<[],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_release                        : Intrinsic<[], [llvm_ptr_ty]>;
 def int_objc_retain                         : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_retainAutorelease              : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_retainAutoreleaseReturnValue   : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_retainAutoreleasedReturnValue  : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_retainBlock                    : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_storeStrong                    : Intrinsic<[],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_storeWeak                      : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_clang_arc_use                  : Intrinsic<[],
                                                         [llvm_vararg_ty]>;
 def int_objc_clang_arc_noop_use : DefaultAttrsIntrinsic<[],
                                                         [llvm_vararg_ty],
                                                         [IntrInaccessibleMemOnly]>;
 def int_objc_unsafeClaimAutoreleasedReturnValue : Intrinsic<[llvm_ptr_ty],
-                                                            [llvm_ptr_ty]>;
+                                                            [llvm_ptr_ty],
+                                                            [Returned<ArgIndex<0>>]>;
 def int_objc_retainedObject                 : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_unretainedObject               : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_unretainedPointer              : Intrinsic<[llvm_ptr_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_retain_autorelease             : Intrinsic<[llvm_ptr_ty],
-                                                        [llvm_ptr_ty]>;
+                                                        [llvm_ptr_ty],
+                                                        [Returned<ArgIndex<0>>]>;
 def int_objc_sync_enter                     : Intrinsic<[llvm_i32_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_sync_exit                      : Intrinsic<[llvm_i32_ty],
                                                         [llvm_ptr_ty]>;
 def int_objc_arc_annotation_topdown_bbstart : Intrinsic<[],
                                                         [llvm_ptr_ty,
                                                          llvm_ptr_ty]>;
 def int_objc_arc_annotation_topdown_bbend   : Intrinsic<[],

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

for (Use &U : llvm::make_early_inc_range(F.uses())) {
     // knowledge of the runtime functions.
     //
     // std::max respects both requirements of notail and tail here:
     // * notail on either the call or from ObjCARC becomes notail
     // * tail on either side is stronger than none, but not notail
     CallInst::TailCallKind TCK = CI->getTailCallKind();
     NewCI->setTailCallKind(std::max(TCK, OverridingTCK));
 
+    // Transfer the 'returned' attribute from the intrinsic to the call site.
+    // By applying this only to intrinsic call sites, we avoid applying it to
+    // non-ARC explicit calls to things like objc_retain which have not been
+    // auto-upgraded to use the intrinsics.
+    unsigned Index;
+    if (F.getAttributes().hasAttrSomewhere(Attribute::Returned, &Index) &&
+        Index)
+      NewCI->addParamAttr(Index - AttributeList::FirstArgIndex,
+                          Attribute::Returned);
+
     if (!CI->use_empty())
       CI->replaceAllUsesWith(NewCI);
     CI->eraseFromParent();
   }
 
   return true;
 }
 

llvm/test/Transforms/PreISelIntrinsicLowering/objc-arc.ll

 ; RUN: opt -pre-isel-intrinsic-lowering -S -o - %s | FileCheck %s
 
 ; Make sure calls to the objc intrinsics are translated to calls in to the
 ; runtime
 
 declare ptr @foo()
 declare i32 @__gxx_personality_v0(...)
 
 define ptr @test_objc_autorelease(ptr %arg0) {
 ; CHECK-LABEL: test_objc_autorelease
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = notail call ptr @objc_autorelease(ptr %arg0)
+; CHECK-NEXT: %0 = notail call ptr @objc_autorelease(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.autorelease(ptr %arg0)
   ret ptr %0
 }
 
 define void @test_objc_autoreleasePoolPop(ptr %arg0) {
 ; CHECK-LABEL: test_objc_autoreleasePoolPop

 entry:
   %0 = call ptr @llvm.objc.autoreleasePoolPush()
   ret ptr %0
 }
 
 define ptr @test_objc_autoreleaseReturnValue(ptr %arg0) {
 ; CHECK-LABEL: test_objc_autoreleaseReturnValue
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = tail call ptr @objc_autoreleaseReturnValue(ptr %arg0)
+; CHECK-NEXT: %0 = tail call ptr @objc_autoreleaseReturnValue(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.autoreleaseReturnValue(ptr %arg0)
   ret ptr %0
 }
 
 define void @test_objc_copyWeak(ptr %arg0, ptr %arg1) {
 ; CHECK-LABEL: test_objc_copyWeak

 entry:
   call void @llvm.objc.release(ptr %arg0)
   ret void
 }
 
 define ptr @test_objc_retain(ptr %arg0) {
 ; CHECK-LABEL: test_objc_retain
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = tail call ptr @objc_retain(ptr %arg0)
+; CHECK-NEXT: %0 = tail call ptr @objc_retain(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.retain(ptr %arg0)
   ret ptr %0
 }
 
 define ptr @test_objc_retainAutorelease(ptr %arg0) {
 ; CHECK-LABEL: test_objc_retainAutorelease
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = call ptr @objc_retainAutorelease(ptr %arg0)
+; CHECK-NEXT: %0 = call ptr @objc_retainAutorelease(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.retainAutorelease(ptr %arg0)
   ret ptr %0
 }
 
 define ptr @test_objc_retainAutoreleaseReturnValue(ptr %arg0) {
 ; CHECK-LABEL: test_objc_retainAutoreleaseReturnValue
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = tail call ptr @objc_retainAutoreleaseReturnValue(ptr %arg0)
+; CHECK-NEXT: %0 = tail call ptr @objc_retainAutoreleaseReturnValue(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = tail call ptr @llvm.objc.retainAutoreleaseReturnValue(ptr %arg0)
   ret ptr %0
 }
 
 define ptr @test_objc_retainAutoreleasedReturnValue(ptr %arg0) {
 ; CHECK-LABEL: test_objc_retainAutoreleasedReturnValue
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = tail call ptr @objc_retainAutoreleasedReturnValue(ptr %arg0)
+; CHECK-NEXT: %0 = tail call ptr @objc_retainAutoreleasedReturnValue(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.retainAutoreleasedReturnValue(ptr %arg0)
   ret ptr %0
 }
 
 define void @test_objc_retainAutoreleasedReturnValue_bundle() {
 ; CHECK-LABEL: test_objc_retainAutoreleasedReturnValue_bundle(

 entry:
   %0 = call ptr @llvm.objc.storeWeak(ptr %arg0, ptr %arg1)
   ret ptr %0
 }
 
 define ptr @test_objc_unsafeClaimAutoreleasedReturnValue(ptr %arg0) {
 ; CHECK-LABEL: test_objc_unsafeClaimAutoreleasedReturnValue
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = tail call ptr @objc_unsafeClaimAutoreleasedReturnValue(ptr %arg0)
+; CHECK-NEXT: %0 = tail call ptr @objc_unsafeClaimAutoreleasedReturnValue(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.unsafeClaimAutoreleasedReturnValue(ptr %arg0)
   ret ptr %0
 }
 
 define void @test_objc_unsafeClaimAutoreleasedReturnValue_bundle() {
 ; CHECK-LABEL: test_objc_unsafeClaimAutoreleasedReturnValue_bundle(

 entry:
   %0 = call ptr @llvm.objc.unretainedPointer(ptr %arg0)
   ret ptr %0
 }
 
 define ptr @test_objc_retain_autorelease(ptr %arg0) {
 ; CHECK-LABEL: test_objc_retain_autorelease
 ; CHECK-NEXT: entry
-; CHECK-NEXT: %0 = call ptr @objc_retain_autorelease(ptr %arg0)
+; CHECK-NEXT: %0 = call ptr @objc_retain_autorelease(ptr returned %arg0)
 ; CHECK-NEXT: ret ptr %0
 entry:
   %0 = call ptr @llvm.objc.retain.autorelease(ptr %arg0)
   ret ptr %0
 }
 
 define i32 @test_objc_sync_enter(ptr %arg0) {
 ; CHECK-LABEL: test_objc_sync_enter