[llvm] 643ce61 - [ObjC][ARC] Don't form a StoreStrong call if it is unsafe to move the

[Akira Hatanaka via llvm-commits]

Author: Akira Hatanaka
Date: 2021-08-11T13:50:19-07:00
New Revision: 643ce61fb3c2c730b7ecead4a489eaeef3f053ea

URL: https://github.com/llvm/llvm-project/commit/643ce61fb3c2c730b7ecead4a489eaeef3f053ea
DIFF: https://github.com/llvm/llvm-project/commit/643ce61fb3c2c730b7ecead4a489eaeef3f053ea.diff

LOG: [ObjC][ARC] Don't form a StoreStrong call if it is unsafe to move the
release call

findSafeStoreForStoreStrongContraction checks whether it's safe to move
the release call to the store by inspecting all instructions between the
two, but was ignoring retain instructions. This was causing objects to
be released and deallocated before they were retained.

rdar://81668577

Added: 
    

Modified: 
    llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
    llvm/test/Transforms/ObjCARC/contract-storestrong.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
index 62161b5b6b40a..577973c806015 100644
--- a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
+++ b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
@@ -226,13 +226,6 @@ static StoreInst *findSafeStoreForStoreStrongContraction(LoadInst *Load,
     // of Inst.
     ARCInstKind Class = GetBasicARCInstKind(Inst);
 
-    // If Inst is an unrelated retain, we don't care about it.
-    //
-    // TODO: This is one area where the optimization could be made more
-    // aggressive.
-    if (IsRetain(Class))
-      continue;
-
     // If we have seen the store, but not the release...
     if (Store) {
       // We need to make sure that it is safe to move the release from its
@@ -248,8 +241,18 @@ static StoreInst *findSafeStoreForStoreStrongContraction(LoadInst *Load,
       return nullptr;
     }
 
-    // Ok, now we know we have not seen a store yet. See if Inst can write to
-    // our load location, if it can not, just ignore the instruction.
+    // Ok, now we know we have not seen a store yet.
+
+    // If Inst is a retain, we don't care about it as it doesn't prevent moving
+    // the load to the store.
+    //
+    // TODO: This is one area where the optimization could be made more
+    // aggressive.
+    if (IsRetain(Class))
+      continue;
+
+    // See if Inst can write to our load location, if it can not, just ignore
+    // the instruction.
     if (!isModSet(AA->getModRefInfo(Inst, Loc)))
       continue;
 

diff  --git a/llvm/test/Transforms/ObjCARC/contract-storestrong.ll b/llvm/test/Transforms/ObjCARC/contract-storestrong.ll
index eff0a6fdf900a..9c45e3334d830 100644
--- a/llvm/test/Transforms/ObjCARC/contract-storestrong.ll
+++ b/llvm/test/Transforms/ObjCARC/contract-storestrong.ll
@@ -256,6 +256,25 @@ define i8* @test13(i8* %a0, i8* %a1, i8** %addr, i8* %new) {
   ret i8* %retained
 }
 
+; Cannot form a storeStrong call because it's unsafe to move the release call to
+; the store.
+
+; CHECK-LABEL: define void @test14(
+; CHECK: %[[V0:.*]] = load i8*, i8** %a
+; CHECK: %[[V1:.*]] = call i8* @llvm.objc.retain(i8* %p)
+; CHECK: store i8* %[[V1]], i8** %a
+; CHECK: %[[V2:.*]] = call i8* @llvm.objc.retain(i8* %[[V0]])
+; CHECK: call void @llvm.objc.release(i8* %[[V2]])
+
+define void @test14(i8** %a, i8* %p) {
+  %v0 = load i8*, i8** %a, align 8
+  %v1 = call i8* @llvm.objc.retain(i8* %p)
+  store i8* %p, i8** %a, align 8
+  %v2  = call i8* @llvm.objc.retain(i8* %v0)
+  call void @llvm.objc.release(i8* %v0)
+  ret void
+}
+
 !0 = !{}
 
 ; CHECK: attributes [[NUW]] = { nounwind }