[PATCH] D103000: [ObjC][ARC] Use the addresses of the ARC runtime functions instead of integer 0/1 for the operand of bundle "clang.arc.attachedcall"

[Akira Hatanaka via Phabricator via llvm-commits]

ahatanak added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1698
         continue;
 
       if (auto *II = dyn_cast<IntrinsicInst>(&*CurI)) {
----------------
ahatanak wrote:
> dexonsmith wrote:
> > If you do an NFC readability patch (see below), I wonder if it would be correct to add a top-level early break here for clarity, something like this?
> > ```
> > lang=c++
> > if (!isa<CallBase>(*CurI))
> >   break;
> > ```
> > (or maybe I missed something?)
> > 
> > Also, should this be ignoring (should it walk past) debug info intrinsics? I'm accustomed to seeing walking code do something like this:
> > ```
> > lang=c++
> > if (isa<DbgInfoIntrinsic>(*CurI))
> >   continue;
> > ```
> > Or does the IR some how guarantee that the debug info intrinsics won't interfere? (if there's a bug, probably the fix should be in a different patch, just want to understand though...)
> > If you do an NFC readability patch (see below), I wonder if it would be correct to add a top-level early break here for clarity, something like this?
> > ```
> > lang=c++
> > if (!isa<CallBase>(*CurI))
> >   break;
> > ```
> > (or maybe I missed something?)
> > 
> 
> Yes, that would be correct.
> 
> > Also, should this be ignoring (should it walk past) debug info intrinsics? I'm accustomed to seeing walking code do something like this:
> > ```
> > lang=c++
> > if (isa<DbgInfoIntrinsic>(*CurI))
> >   continue;
> > ```
> > Or does the IR some how guarantee that the debug info intrinsics won't interfere? (if there's a bug, probably the fix should be in a different patch, just want to understand though...)
> 
> No, it doesn't guarantee that there are no debug instructions. It should be skipped.
> 
> This code is trying to see whether we can move the autoreleaseRV call or the normal call towards the return instruction, so we should be able to skip `CurI` if it wouldn't prevent moving those calls towards the return.
> 
> If we decide to change this code, we should do so in a separate patch.
I'm thinking about making the following changes to the code in the inliner in the future:

1. Ignore other instructions, such as debug instructions, that don't interfere with this optimization.
2. If pairing up the attached retainRV/claimRV call in the caller with the autoreleaseRV call in the callee fails, emit a call to retainRV or claimRV instead of emitting a call to retain or emitting nothing. `OptimizeInlinedAutoreleaseRVCall` in ObjCARCOpts.cpp, which is run later, might still succeed in pairing up the calls when the inliner fails.
3. Move `OptimizeReturn` in ObjCARCOpts.cpp, which eliminates retainRV/autoreleaseRV pairs in the return blocks of the callee and is currently run before the code here, to ObjCARCContract.cpp. This will simplify the code here as we can just look for an autoreleaseRV call and won't have to look for an unannotated call.
4. Be less conservative when pairing up retainRV/claimRV calls with autoreleaseRV calls (see the discussion in https://reviews.llvm.org/D70370). Also, delete `OptimizeInlinedAutoreleaseRVCall` if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103000