Page MenuHomePhabricator

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

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.Mon, Jul 5, 2:16 AM
llvm/include/llvm/Analysis/ObjCARCUtil.h
29

Perhaps simpler to return an optional?

llvm/lib/Target/X86/X86ISelLowering.cpp
4440–4441

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–1680

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.Thu, Jul 8, 12:44 PM
ahatanak marked 2 inline comments as done.

Rebase and address review comments.

llvm/lib/Target/X86/X86ISelLowering.cpp
4440–4441

I replaced the number with a target global address.

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

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
2417–2419

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
4438–4439

Comment (RuntimeCallType) seems out of date now.

4443–4447

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–1680

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?

1698

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

1700–1702

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.

1719

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

1719–1721

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
1738

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

  • Invert the condition and add an early continue
1956–1958

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.Thu, Jul 8, 6:59 PM
llvm/docs/LangRef.rst
2417–2419

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
4443–4447

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
1698

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.

1719

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.

1956–1958

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

dexonsmith added inline comments.Fri, Jul 9, 11:02 AM
llvm/docs/LangRef.rst
2417–2419

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?