This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ahatanak on May 23 2021, 8:31 PM.

Details

Summary

https://reviews.llvm.org/D102996 changes the operand of bundle "clang.arc.attachedcall". This patch makes changes to llvm that are needed to handle the new IR.

This should make it easier to understand what the IR is doing and also simplify some of the passes as they no longer have to translate the integer values to the runtime functions.

Diff Detail

Event Timeline

ahatanak created this revision.May 23 2021, 8:31 PM
ahatanak requested review of this revision.May 23 2021, 8:31 PM
fhahn added inline comments.Jul 5 2021, 2:16 AM
llvm/include/llvm/Analysis/ObjCARCUtil.h
30

Perhaps simpler to return an optional?

llvm/lib/Target/X86/X86ISelLowering.cpp
4630–4631

Would to be possible to add an operand with the symbol of the function instead a number here?

llvm/lib/Transforms/Utils/InlineFunction.cpp
1677–1678

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?

ahatanak updated this revision to Diff 357324.Jul 8 2021, 12:44 PM
ahatanak marked 2 inline comments as done.

Rebase and address review comments.

llvm/lib/Target/X86/X86ISelLowering.cpp
4630–4631

I replaced the number with a target global address.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1677–1678

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.

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?

llvm/docs/LangRef.rst
2477–2479

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?

llvm/lib/Target/X86/X86ISelLowering.cpp
4630–4631

Comment (RuntimeCallType) seems out of date now.

4633–4637

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?
llvm/lib/Transforms/Utils/InlineFunction.cpp
1673–1675

I wonder if this change to the signature is needed. Not sure if it's preparing for another patch...

1677–1678

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:

assert(RVCallKind && "Expected an attached ARC function");

... but maybe you still want the full assertion here for clarity?

1696

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?

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:

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...)

1697–1698

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.

1697–1698

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 ifs and early breaks instead
1697–1698

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...)

1742

If you do an NFC prep patch to reduce nesting, then I suggest:

  • Invert the condition and add an early continue
1967–1970

Is this change necessary? Or could the call to getAttachedARCFunctionKind happen inside inlineRetainOrClaimRVCalls?

llvm/test/Verifier/operand-bundles.ll
68

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.

ahatanak added inline comments.Jul 8 2021, 6:59 PM
llvm/docs/LangRef.rst
2477–2479

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"()).

llvm/lib/Target/X86/X86ISelLowering.cpp
4633–4637

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.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1696

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?

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:

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.

1697–1698

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.

1967–1970

I changed this just to avoid calling both hasAttachedCallOpBundle and getAttachedARCFunctionKind since both of them call getOperandBundle.

dexonsmith added inline comments.Jul 9 2021, 11:02 AM
llvm/docs/LangRef.rst
2477–2479

Thanks for explaining / sorry for missing this! I had misunderstood this as an explicit operand called null.

Can you add a couple of examples of using the operand bundle, demonstrating the three correct uses in a code-block?

ahatanak updated this revision to Diff 367660.Aug 19 2021, 4:48 PM
ahatanak marked 6 inline comments as done.

Address review comments.

ahatanak added inline comments.Aug 19 2021, 5:13 PM
llvm/lib/Transforms/Utils/InlineFunction.cpp
1696

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.
ahatanak updated this revision to Diff 369770.Aug 31 2021, 1:04 PM

Rebase and ping.

The IR parts of this LGTM. I don't feel qualified to review the backend.

fhahn accepted this revision.Sep 1 2021, 8:21 AM

Backend/middle-end changes LGTM, thanks for all the cleanups! It might be good to wait a day or two with committing, so @dexonsmith has a chance to see if there are any additional suggestions.

This revision is now accepted and ready to land.Sep 1 2021, 8:21 AM
This revision was landed with ongoing or failed builds.Sep 8 2021, 12:02 PM
This revision was automatically updated to reflect the committed changes.