[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"

Thu Jul 8 15:05:30 PDT 2021

dexonsmith added a comment.

It's great to see this cleanup!

I have a few suggestions / questions inline. Also, should there be a bitcode upgrade to convert `i64 0` and `i64 1` over to the runtime functions?

================
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
----------------
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?

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4438-4439
     // Add target constant to select ObjC runtime call just before the call
     // target. RuntimeCallType == 0 selects objc_retainAutoreleasedReturnValue,
     // RuntimeCallType == 0 selects objc_unsafeClaimAutoreleasedReturnValue when
     // epxanding the pseudo.
----------------
Comment (RuntimeCallType) seems out of date now.

================
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");
----------------
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?

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1673-1675
 static void
-inlineRetainOrClaimRVCalls(CallBase &CB,
+inlineRetainOrClaimRVCalls(CallBase &CB, objcarc::ARCInstKind RVCallKind,
                            const SmallVectorImpl<ReturnInst *> &Returns) {
----------------
I wonder if this change to the signature is needed. Not sure if it's preparing for another patch...

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1698
         continue;

       if (auto *II = dyn_cast<IntrinsicInst>(&*CurI)) {
----------------
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...)

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1700-1702
         if (II->getIntrinsicID() == Intrinsic::objc_autoreleaseReturnValue &&
             II->hasNUses(0) &&
             objcarc::GetRCIdentityRoot(II->getOperand(0)) == RetOpnd) {
----------------
If you're up for it, it'd be nice to commit an NFC prep patch to improve readability / reduce nesting, then rebase this on top. Here, you can invert the condition and add an early `break`.

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1719
         }
       } else if (auto *CI = dyn_cast<CallInst>(&*CurI)) {
         if (objcarc::GetRCIdentityRoot(CI) == RetOpnd &&
----------------
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...)

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1719-1721
       } else if (auto *CI = dyn_cast<CallInst>(&*CurI)) {
         if (objcarc::GetRCIdentityRoot(CI) == RetOpnd &&
             !objcarc::hasAttachedCallOpBundle(CI)) {
----------------
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

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1738

     if (InsertRetainCall) {
       // The retainRV is attached to the call and we've failed to find a
----------------
If you do an NFC prep patch to reduce nesting, then I suggest:
- Invert the condition and add an early `continue`

================
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);
----------------
Is this change necessary? Or could the call to getAttachedARCFunctionKind happen inside `inlineRetainOrClaimRVCalls`?

================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1675
   Module *Mod = CB.getModule();
-  bool IsRetainRV = objcarc::hasAttachedCallOpBundle(&CB, true),
+  assert(((RVCallKind == objcarc::ARCInstKind::RetainRV) ||
+          (RVCallKind == objcarc::ARCInstKind::ClaimRV)) &&
----------------
ahatanak wrote:
> fhahn wrote:
> > Would it make sense to have a function in the ‘obj arc’’ namespace to check IsRetainRV instead of duplicating the check + assertion in multiple paces?
> I'm not sure how you can remove code duplication using `isRetainRV`, which just checks whether the function is `objc_retainAutoreleasedReturnValue`. Did you mean `isRetainOrClaimRV`, which checks whether the function is either `objc_retainAutoreleasedReturnValue` or `objc_unsafeClaimAutoreleasedReturnValue`? Also, we can't use anything from `objcarc` in Verifier.cpp as that would be a layering violation, so we'd need a separate helper function there.
If you revert the change to the function signature, then the call to `getAttachedARCFunctionKind()` will happen right here -- there's no concern that `RVCallKind` is an arbitrary value since the helper has an assertion.

Maybe then this assertion could just be:
```
lang=c++
assert(RVCallKind && "Expected an attached ARC function");
```
... but maybe you still want the full assertion here for clarity?

================
Comment at: llvm/test/Verifier/operand-bundles.ll:68

 define void @f_clang_arc_attachedcall() {
 ; CHECK: Multiple "clang.arc.attachedcall" operand bundles
----------------
I suggest adding a few more negative testcases with attached calls that point at invalid things to make sure the verifier catches them:
- An arc intrinsic with the right signature that's invalid to use in `clang.arc.attachedcall`
- An arc intrinsic with the wrong signature (also invalid)
- A non-intrinsic with the right signature
- A non-intrisnic with the wrong signature, with or without a bitcast
- `i8* (i8*)* null` (or is this valid? LangRef makes it sound valid but I'm not seeing tests...)
- `i64 0`

I don't see a positive case for the "claim" version... if that's missing, it'd be good to add that too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103000