Page MenuHomePhabricator

Add objc_retain and objc_release intrinsics and codegen them to their runtime methods
ClosedPublic

Authored by pete on Dec 3 2018, 1:04 PM.

Details

Summary

This is the first in a series of patches to move objc retain/release to intrinsics and have the ARC optimizer work on those instead.

For some context, see clang r263607 which converted [a retain] to objc_retain(a). That was ultimately reverted as the ARC optimizer couldn't handle the new calls.

The eventual solution is to move the ARC inserted retain/release calls to intrinsics and only optimize the intrinsics. Manually inserted retain/release will then no longer be optimized.

Diff Detail

Repository
rL LLVM

Event Timeline

pete created this revision.Dec 3 2018, 1:04 PM

Can you add these intrinsics to the LangRef? Seems pretty reasonable, but I don't feel comfortable LGTMing.

include/llvm/IR/Intrinsics.td
324 ↗(On Diff #176459)

I think it would be good to have these for all the entry points used by the arc optimizer, even if it isn't necessary to fix r263607. I guess it doesn't make sense to block this though, but maybe add a FIXME if you agree?

ahatanak added inline comments.Dec 3 2018, 4:37 PM
include/llvm/IR/Intrinsics.td
324 ↗(On Diff #176459)

Would it be possible to add a few comments that explain the motivation for these intrinsics?

Also, r294872 added attribute "returned" to some of the ObjC ARC functions, but was later reverted because of a mis-compile caused by a bug in the ARC optimizer. The ARC optimizer bug that caused the mis-compile hasn't been fixed yet, so I wonder whether it's safe to add "Returned<0>" here.

Also, are there any other properties (IntrArgMemOnly, for example) you can use here?

pete updated this revision to Diff 176529.Dec 3 2018, 6:34 PM
pete marked 2 inline comments as done.

Updated to add a comment to Intrinsics.td, remove the ReturnedValue<> and added all of the ARC methods from https://clang.llvm.org/docs/AutomaticReferenceCounting.html#runtime-support

This revision is now accepted and ready to land.Dec 4 2018, 10:04 AM
pete added a comment.Dec 4 2018, 11:23 AM

LGTM

Thanks!

Erik, did you have anything else required before landing this?

include/llvm/IR/Intrinsics.td
324 ↗(On Diff #176459)

Yeah, i was torn about doing that or not. I think its a good idea so let me try now and see how much work it is.

Certainly it reduces the churn on the next patch which will actually move the ARC optimizer to use the new methods.

324 ↗(On Diff #176459)

Ah yeah will add comments.

I was unsure about returned<0> too as it may cause issues. Good idea to remove it for now.

I don't think we can add IntrArgMemOnly or similar as if these methods are overridden they can have arbitrary side effects.

LGTM

Thanks!

Erik, did you have anything else required before landing this?

Patch LGTM, but are you planning on adding these to the LangRef? Its probably good enough to just list them and refer to https://clang.llvm.org/docs/AutomaticReferenceCounting.html#runtime-support for their semantics.

pete added a comment.Dec 5 2018, 9:06 AM

Discovered that objc_storeStrong is actually wrong in the documentation. Changing it from returning id to returning void. Will also update the docs.

This revision was automatically updated to reflect the committed changes.