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

Thu Feb 11 05:12:20 PST 2021

fhahn added a comment.

In D92808#2555868 <https://reviews.llvm.org/D92808#2555868>, @ahatanak wrote:

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

+1, especially passing the function to call as argument to the bundle would make the lowering slightly easier; at the moment we may need to inject a new declaration during codegen.

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

The patch already tries to do that, by adding calls to `llvm.objc.clang.arc.noop.use`, to make it explicit that there is a use of the returned value. This ensures most optimizations are aware of the fact that the return value is used. The problem Akira is that regular uses can be updated and are not enough to block the problematic transformation in SCCP.

If the use is directly at the bundle,  I think we could somehow teach to lowering to use the referenced value and pass that to the ObjC runtime. But it would disable the runtime optimization.

I think in the short-term it would be OK to update `SCCP` as done in this patch, if we specify the `arc` bundle implicitly uses the return value, which does not refer to any ObjC specifics. We already do something similar for function with certain attributes.

I think it would be good to consider generalizing this 'implicitly used return value bundle' concept as a follow up, if there are other potential users. We then already laid the ground-work and updated the existing passes to work for the `arc` bundle and only have to update the checks.

@dexonsmith  @rjmccall What do you think? Would you be happy with iterating on the suggestions in tree?

================
Comment at: llvm/docs/LangRef.rst:2335
+``@objc_retainAutoreleasedReturnValue`` is called. If 1 is passed,
+``@objc_unsafeClaimAutoreleasedReturnValue`` is called.
+
----------------
Can you add a sentence making it explicit that calls with that bundle implicitly use the returned value?

================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:148
+  /// A list of functions whose return values cannot be zapped.
+  SmallPtrSet<Function *, 16> DontZapReturnFunctions;

----------------
I think it would be slightly simpler to have a positive name for the set, e.g. `MustPreserveReturnsInFn`. What do you think? IMO that's more general than referring to 'not zapping'.

================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1643
+  // unless the call itself can be removed.
+  // If the call has operand bundle "clang.arc.rv", don't replace the call value
+  // with a constant. Doing so would enable dead argument elimination to change
----------------
I'd keep things generic and say something like `Calls with "clang.arc.rv" implicitly use the return value and those uses cannot be updated with a constant.`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92808