[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/docs/LangRef.rst:2417-2419
+``@objc_unsafeClaimAutoreleasedReturnValue``) or null. A null operand indicates
+the marker instruction has to be emitted after the call but calls to the runtime
+functions don't have to be emitted since they already have been emitted. The
----------------
dexonsmith wrote:
> This "null operand" sentence seems new in LangRef, but I'm not seeing tests / code changes for it. Is this just fixing a documentation bug, or are you intending this for a different patch?
The operand bundle argument is removed in `BundledRetainClaimRV::~BundledRetainClaimRV` when ARC contract pass is being run. That is the code change. The test cases for that change are in llvm/test/Transforms/ObjCARC (for example, contract-rv-attr.ll). The tests check that the operand bundles have null operands (`"clang.arc.attachedcall"()`).


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4443-4447
+    assert(
+        ARCFn &&
+        (ARCFn == M->getFunction("objc_retainAutoreleasedReturnValue") ||
+         ARCFn == M->getFunction("objc_unsafeClaimAutoreleasedReturnValue")) &&
+        "unexpected ARC function");
----------------
dexonsmith wrote:
> A few thoughts on this:
> 
> - Seems like there are really two assertions here, which might deserve different messages:
>   - `assert(ArcFn && "expected attached ARC function")`
>   - `assert(is-valid-attached-arc-call && "invalid attached ARC function")`
> 
> - It'd be good to confirm that the verifier catches any problems like this before the assertion gets hit; I've made some suggestions in the verifier test for negative cases to add.
> 
> - Is there a convenient way / place to encapsulate the is-valid-attached-arc-call check? One possibility would be to move that part of the assertion to `getAttachedARCFunction()`, but maybe that'd cause a problem for the verifier, not sure but in that case maybe another function in `objcarc::`.
> 
> - `M->getFunction()` does a hash table lookup... if just the name matters, could you just do `ARCFn->getName()`? or if you're also validating ownership, can you check `ARCFn->getParent() == M`, which is constant time, instead of looking up in the hash table?
> A few thoughts on this:
> 
> - Seems like there are really two assertions here, which might deserve different messages:
>   - `assert(ArcFn && "expected attached ARC function")`
>   - `assert(is-valid-attached-arc-call && "invalid attached ARC function")`
> 
> - It'd be good to confirm that the verifier catches any problems like this before the assertion gets hit; I've made some suggestions in the verifier test for negative cases to add.
>

Yes, I think the verifier should check that the attached-call bundle takes pointers to the expected runtime non-intrinsic functions. Then the check here might not be needed anymore.

> - Is there a convenient way / place to encapsulate the is-valid-attached-arc-call check? One possibility would be to move that part of the assertion to `getAttachedARCFunction()`, but maybe that'd cause a problem for the verifier, not sure but in that case maybe another function in `objcarc::`.
>

I think there should be a function in `objcarc::`, but we also need another helper function in the verifier since we can't use anything in `objcarc::` in the verifier.
 
> - `M->getFunction()` does a hash table lookup... if just the name matters, could you just do `ARCFn->getName()`? or if you're also validating ownership, can you check `ARCFn->getParent() == M`, which is constant time, instead of looking up in the hash table?

We can just compare the names here.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1698
         continue;
 
       if (auto *II = dyn_cast<IntrinsicInst>(&*CurI)) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1719
         }
       } else if (auto *CI = dyn_cast<CallInst>(&*CurI)) {
         if (objcarc::GetRCIdentityRoot(CI) == RetOpnd &&
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > If you do an NFC prep patch to reduce nesting, then I suggest:
> > - Put a `break` before the `}` and drop the `else`
> > - Invert the conditions on the `if`s and early `break`s instead
> Is there a reason this doesn't use `CallBase`, or otherwise deal with `InvokeInst`? (if there's a bug, probably the fix should be in a different patch, just want to understand though...)
This code doesn't expect to see any terminator instructions like `InvokeInst` as it iterates over the instructions in the basic block backwards starting at the instruction before the return instruction. It doesn't visit any of the predecessor blocks although we might find out later that we need to do so in some cases.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1956-1958
+    objcarc::ARCInstKind RVCallKind = objcarc::getAttachedARCFunctionKind(&CB);
+    if (RVCallKind != objcarc::ARCInstKind::None)
+      inlineRetainOrClaimRVCalls(CB, RVCallKind, Returns);
----------------
dexonsmith wrote:
> Is this change necessary? Or could the call to getAttachedARCFunctionKind happen inside `inlineRetainOrClaimRVCalls`?
I changed this just to avoid calling both `hasAttachedCallOpBundle` and `getAttachedARCFunctionKind` since both of them call `getOperandBundle`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103000