ObjCARCContract: Use stripPointerCastsAndAliases

[arsenm]

No description provided.

[arsenm]

• IR: Remove uselist for constantdata #134692
• SCEVExpander: Don't look at uses of constants #134691
• LICM: Avoid looking at use list of constant data #134690
• PHITransAddr: Avoid looking at constant use lists #134689
• SimplifyLibCalls: Skip sincospi optimization for ConstantData #134688
• ObjCARCContract: Use stripPointerCastsAndAliases #134275 👈 (View in Graphite)
• ObjCARC: Drop pointer bitcast handling #134274
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Matt Arsenault (arsenm)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/134275.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp (+10-24)

diff --git a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
index e11748b2c9dbb..ed7a235f5e2e3 100644
--- a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
+++ b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
@@ -661,30 +661,16 @@ bool ObjCARCContract::run(Function &F, AAResults *A, DominatorTree *D) {
 
     Value *Arg = cast<CallInst>(Inst)->getArgOperand(0);
 
-    // TODO: Change this to a do-while.
-    for (;;) {
-      ReplaceArgUses(Arg);
-
-      // If Arg is a no-op casted pointer, strip one level of casts and iterate.
-      if (const BitCastInst *BI = dyn_cast<BitCastInst>(Arg))
-        Arg = BI->getOperand(0);
-      else if (isa<GEPOperator>(Arg) &&
-               cast<GEPOperator>(Arg)->hasAllZeroIndices())
-        Arg = cast<GEPOperator>(Arg)->getPointerOperand();
-      else if (isa<GlobalAlias>(Arg) &&
-               !cast<GlobalAlias>(Arg)->isInterposable())
-        Arg = cast<GlobalAlias>(Arg)->getAliasee();
-      else {
-        // If Arg is a PHI node, get PHIs that are equivalent to it and replace
-        // their uses.
-        if (PHINode *PN = dyn_cast<PHINode>(Arg)) {
-          SmallVector<Value *, 1> PHIList;
-          getEquivalentPHIs(*PN, PHIList);
-          for (Value *PHI : PHIList)
-            ReplaceArgUses(PHI);
-        }
-        break;
-      }
+    ReplaceArgUses(Arg);
+
+    Arg = Arg->stripPointerCastsAndAliases();
+    // If Arg is a PHI node, get PHIs that are equivalent to it and replace
+    // their uses.
+    if (PHINode *PN = dyn_cast<PHINode>(Arg)) {
+      SmallVector<Value *, 1> PHIList;
+      getEquivalentPHIs(*PN, PHIList);
+      for (Value *PHI : PHIList)
+        ReplaceArgUses(PHI);
     }
   }
 

[arsenm]

ping

[ahatanak]

Is this supposed to be a NFC patch?

With the changes in this patch, the pass no longer rewrites the argument to the second call to use_pointer in the following IR:

define void @test0(ptr %x) {
  %y = getelementptr inbounds ptr, ptr %x, i32 0
  %v0 = call ptr @llvm.objc.retain(ptr %y) nounwind
  call void @use_pointer(ptr %x)
  call void @use_pointer(ptr %y)
  ret void
}

This is a contrived example and maybe it's okay if we know that clang/llvm never generates IR like this in practice. But I'd like to understand what assumptions you are making about the IR that's fed to this pass.

[ahatanak]

Correction: with the changes in this patch, the pass no longer rewrites the argument to the first call to use_pointer in the following IR:

[arsenm]

Is this supposed to be a NFC patch?

Yes. This is code that's plainly reinventing a common helper function

With the changes in this patch, the pass no longer rewrites the argument to the second call to use_pointer in the following IR:

define void @test0(ptr %x) {
  %y = getelementptr inbounds ptr, ptr %x, i32 0
  %v0 = call ptr @llvm.objc.retain(ptr %y) nounwind
  call void @use_pointer(ptr %x)
  call void @use_pointer(ptr %y)
  ret void
}

Missing test coverage. I'm assuming this loop is not properly tested. Can you push this to fix the gaps?

This is a contrived example and maybe it's okay if we know that clang/llvm never generates IR like this in practice. But I'd like to understand what assumptions you are making about the IR that's fed to this pass.

My only assumption is the lit test suite is adequate to cover all of the functionality exercised in the code

[nikic]

I'm not convinced that the replacement with stripPointerCastsAndAliases() makes sense. Yes, this does something similar, but the important difference is that the current code performs the replacement at each instruction in the chain. Now you are only doing it at the first and last value in the chain, which is not the same.

[arsenm]

Don't care about this to spend any time on it