This is an archive of the discontinued LLVM Phabricator instance.

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

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 ↗(On Diff #364215)

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.Aug 30 2021, 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.Aug 30 2021, 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.

jroelofs updated this revision to Diff 545752.Jul 31 2023, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 11:48 AM
nikic requested changes to this revision.Jul 31 2023, 11:58 AM
nikic added a subscriber: nikic.

Please separate the change to stripPointerCasts() into a separate review.

This revision now requires changes to proceed.Jul 31 2023, 11:58 AM
jroelofs updated this revision to Diff 545779.Jul 31 2023, 1:31 PM

Move the stripPointerCasts() change into its own review: https://reviews.llvm.org/D156735

oops, uploaded the wrong patch to this review.

nikic resigned from this revision.Jul 31 2023, 1:53 PM
This revision is now accepted and ready to land.Jul 31 2023, 1:53 PM
jroelofs updated this revision to Diff 546219.Aug 1 2023, 2:13 PM

Rebase. Avoid the dependency on D156735