[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

Thanks for the updates! The LLVM side of things now looks good to me. Could you also add a description of the new bundle to LangRef https://llvm.org/docs/LangRef.html#operand-bundles?

In D92808#2535593 <https://reviews.llvm.org/D92808#2535593>, @ahatanak wrote:

> We could run another pass to clean up the IR after inlining, but I think it's better to do the cleanup right after the callee function is cloned in the caller function.

Thanks for elaborating. Given that it is isolated to a single place and the inliner already has logic to deal with a few different intrinsics/bundle types, I think that should be fine.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:449
                                                         [llvm_vararg_ty]>;
+def int_objc_clang_arc_noop_use             : Intrinsic<[],
+                                                        [llvm_vararg_ty],
----------------
ahatanak wrote:
> fhahn wrote:
> > Can this be a `DefaultAttrIntrinsics` (which adds ` nofree nosync nounwind willreturn`)?
> > 
> > Same probably for all/most objc_* intrinsics.
> I'll change the other intrinsics in a separate patch.
SGTM. thanks!


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1655
+///
+/// 1. If there is a call to autoreleasaeRV that takes a pointer to the returned
+///    object in the callee return block, the autoreleaseRV call and the
----------------
nit: typo `autoreleasaeRV`


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1669
+                           const SmallVectorImpl<ReturnInst *> &Returns) {
+  Module *Mod = CB.getParent()->getParent()->getParent();
+  bool IsRetainRV = objcarc::hasRVOpBundle(&CB, true), IsClaimRV = !IsRetainRV;
----------------
nit: Can you just use `CB.getModule()`?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1673
+  for (auto *RI : Returns) {
+    Value *RetOpnd = llvm::objcarc::GetRCIdentityRoot(RI->getOperand(0));
+    BasicBlock::reverse_iterator I = ++(RI->getIterator().getReverse());
----------------
we are in the `llvm::` namespace here I think, so this should not be needed ? above you use the `objcarc` namespace without `llvm::` prefix as well. Same in other places in the function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92808/new/

https://reviews.llvm.org/D92808