Page MenuHomePhabricator

[Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.
AcceptedPublic

Authored by jroelofs on Jul 8 2021, 4:25 PM.

Diff Detail

Event Timeline

jroelofs created this revision.Jul 8 2021, 4:25 PM
jroelofs requested review of this revision.Jul 8 2021, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 4:25 PM
jroelofs updated this revision to Diff 359364.Jul 16 2021, 9:46 AM

clang-format

IIUC, we sometimes turn -retain message sends into calls to objc_retain and so on. Outside of ARC (the only time they're valid), these calls don't have the semantics of guaranteeing to return their self argument. So I think this change is only fine if we're not applying it when doing that non-ARC transformation.

jroelofs updated this revision to Diff 359478.Jul 16 2021, 4:35 PM

Apply the attribute only to call sites emitted via calls to the intrinsics.

jroelofs updated this revision to Diff 359482.Jul 16 2021, 4:45 PM

Explain why the attribute goes on the call site and not the callee.

jroelofs updated this revision to Diff 364215.Aug 4 2021, 12:24 PM

Add a note/check to the [x retain] => objc_retain(x) test explaining why it shouldn't be lowered as an intrinsic call in the future.

Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 12:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

IIUC, we sometimes turn -retain message sends into calls to objc_retain and so on. Outside of ARC (the only time they're valid), these calls don't have the semantics of guaranteeing to return their self argument. So I think this change is only fine if we're not applying it when doing that non-ARC transformation.

I believe this is covered now. The -retain => objc_retain transformation emits normal function calls, and objc_retain calls are only transformed into intrinsic calls when ARC is enabled.

rjmccall accepted this revision.Aug 23 2021, 8:53 PM

Okay, thanks, seems like a good approach.

clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
34

This only actually tests for the absence of these lines between the CHECK-LABEL and the first CALLS line below, so I don't think it does much.

This revision is now accepted and ready to land.Aug 23 2021, 8:53 PM
jroelofs updated this revision to Diff 369584.Mon, Aug 30, 5:12 PM

Rebased.

Also, turns out that stripPointerCasts() can see through the thisreturn attribute, which defeats a self retain optimization, breaking one of the clang tests. I tweaked a callback to make it configurable. Let me know if you see a better way of dealing with that.

jroelofs marked an inline comment as done.Mon, Aug 30, 5:17 PM

How does stripPointerCasts "see through" an attribute on the underlying call instruction?

How does stripPointerCasts "see through" an attribute on the underlying call instruction?

https://github.com/llvm/llvm-project/blob/bf77b11277411f6725cf09a66feb36d2c14bc8a7/llvm/lib/IR/Value.cpp#L652

Whoa. That does not seem like it's in the contract for stripPointerCasts() generically. At best that should be enabled by the strip kind.