[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

[Akira Hatanaka via Phabricator via llvm-commits]

ahatanak reopened this revision.
ahatanak added a comment.
This revision is now accepted and ready to land.

In D92808#2555737 <https://reviews.llvm.org/D92808#2555737>, @dexonsmith wrote:

> In D92808#2552354 <https://reviews.llvm.org/D92808#2552354>, @rjmccall wrote:
>
>> The ultimate code-generation consequences of this feature aren't very generic, so we shouldn't let ourselves get caught up designing an extremely general feature that ultimately either doesn't do what we need it to do or doesn't actually work for any of the generalizations.
>
> Sure, those are two bad potential outcomes.
>
> Stepping back, the goal of this patch is to encode ARC's code generation invariants in the IR so that maintainers don't inadvertently break them. I think the IR should describe those invariants as clearly and precisely as is reasonable.
>
> Besides idle / misplaced optimism about reuse, I was trying to make the following points along those lines, so I'll restate them more directly:
>
> - Explicitly adding a use of the call avoids having to teach transformations / analyses about it. It might be simpler to add a self-reference carve-out for this operand bundle (or operand bundles in general) than to teach all the passes that this operand bundle implies a use of the callsite (but I'm not sure; maybe there are lots of assumptions about self-references not existing outside of PHIs...).
> - Passing the function that's implicitly called into the operand bundle is simpler to reason about than an opaque table index.
> - Naming the operand bundle after its code generation consequences makes it simpler reason about. (A few generic names that make some sense to me (besides what I listed yesterday) include "callafter", "callimmediately", "forwardto", and "attachedcall". If it's important to make this ARC only, it could be (e.g.) "clang.arc.attachedcall".)

The second and third suggestions seem like an improvement over the bundle used in this patch.

I'm not sure about the first one though as it seems to me that you'll still have to teach at least some of the passes about the self-reference.

For example, if SCCP just does a normal RAUW on the following call, which is taken from the example I posted,

  %r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* %r) ]

will become

  %r = call i8* @foo() [ "clang.arc.rv"(i64 1, i8* @g0) ]

`%r` doesn't have an explicit use in the IR anymore, so passes like deadargelim can change the return type of the function to `void`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92808/new/

https://reviews.llvm.org/D92808