[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 update! The approach with operand bundles and the no-op uses looks cleaner than the original version. The special handling in the inliner seems a bit unfortunate, but I guess there's no way around that?

(as a side note, it might be easier to submit if this would be split up in the changes adding the bundle, followed by the ARC optimization & clang changes)



================
Comment at: llvm/include/llvm/Analysis/ObjCARCUtil.h:41
+static inline bool hasRVOpBundle(const CallBase *CB) {
+  return CB->getOperandBundle(LLVMContext::OB_clang_arc_rv).hasValue();
+}
----------------
I think we should include `LLVMContext.h` explicitly?


================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1217
 
+  static CallBase *addOperandBundle(CallBase *CB, uint32_t ID,
+                                    OperandBundleDef OB,
----------------
Please add documentation for the new functions.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:449
                                                         [llvm_vararg_ty]>;
+def int_objc_clang_arc_noop_use             : Intrinsic<[],
+                                                        [llvm_vararg_ty],
----------------
Can this be a `DefaultAttrIntrinsics` (which adds ` nofree nosync nounwind willreturn`)?

Same probably for all/most objc_* intrinsics.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1650
+static void
+insertRetainOrClaimRVCalls(CallBase &CB,
+                           const SmallVectorImpl<ReturnInst *> &Returns) {
----------------
Could you add a comment elaborating what this function does and why it is needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92808