Page MenuHomePhabricator

[ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR
Needs ReviewPublic

Authored by ahatanak on Dec 7 2020, 7:15 PM.

Details

Summary

Background:

This fixes a longstanding problem where llvm breaks ARC's autorelease optimization (see the link below) by separating calls from the marker instructions or retainRV/claimRV calls.

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-runtime-objc-autoreleasereturnvalue

What this patch does to fix the problem:

  • The front-end annotates calls with attribute retainRV/claimRV, which indicates the call result is implicitly consumed by a retainRV/claimRV call, and attribute rv_marker, which indicates a marker instruction should immediately follow the call. This is currently done only when the target is arm64, the optimization level is higher than -O0, and the OS isn't windows.
  • ARC optimizer temporarily emits retainRV/claimRV calls in the IR if it finds a call annotated with the attributes and removes the inserted calls after processing the function.
  • ARC contract pass emits the retainRV/claimRV calls in the IR and removes the retainRV/claimRV attribute from the call, but leaves the rv_marker attribute on the call. This is done late in the pipeline to prevent optimization passes from transforming the IR in a way that makes it harder for the ARC middle-end passes to figure out the def-use relationship between the call and the retainRV/claimRV calls (which is the cause of PR31925).
  • The function inliner removes an autorelease call in the callee if nothing in the callee prevents it from being paired up with the retainRV/claimRV call in the caller. It then inserts a release call if the call is annotated with claimRV since claimRV+autorelease is equivalent to a release. If it cannot find an autorelease call, it tries to transfer the attributes to a function call in the callee. This is important since ARC optimizer (ObjCARCOpt::OptimizeReturns) removes a retainRV/autorelease pair in the callee if it consumes the result of a tail call and the result is returned by the callee function. If that fails, it simply emits a retain call in the IR if the call is annotated with retainRV and does nothing if it's annotated with claimRV.
  • This patch teaches dead argument elimination pass not to change the return type of a function if any of the calls to the function are annotated with a retainRV/claimRV attribute. This is necessary since the pass can change the return type to void if it determines nothing in the IR uses the function return, which can happen since the front-end no longer emits retainRV/claimRV instructions in the IR that explicitly use the function return.
  • The backend emits a pseudo instruction when it sees a call annotated with rv_marker and expands it later in the pipeline to a bundled instruction (see https://reviews.llvm.org/D92569).

Future work:

  • Use the attribute on x86-64.
  • Fix the auto upgrader to convert call+retainRV/claimRV pairs into calls annotated with the attributes.

rdar://problem/32902328

Diff Detail

Event Timeline

ahatanak created this revision.Dec 7 2020, 7:15 PM
ahatanak requested review of this revision.Dec 7 2020, 7:15 PM
smeenai resigned from this revision.Dec 7 2020, 7:17 PM
smeenai added a reviewer: compnerd.
smeenai added a subscriber: compnerd.

I'm super excited to see this happen, but it's also well out of my review comfort zone, unfortunately. Adding @compnerd to take a look.

smeenai added a subscriber: smeenai.Dec 7 2020, 7:17 PM

Your patch description doesn't mention the changes to the inliner.

clang/lib/CodeGen/CGObjC.cpp
2282

This is too generic for a global variable name.

It should also be const.

2330

It would be good to explain why this is target-specific in a comment.

2336

Didn't we just add this? Can that fail to add the marker?

ahatanak updated this revision to Diff 310387.Dec 8 2020, 5:09 PM
ahatanak marked 2 inline comments as done.
ahatanak edited the summary of this revision. (Show Details)
ahatanak added inline comments.
clang/lib/CodeGen/CGObjC.cpp
2330

I didn't really have a reason for limiting this to arm64 and non-windows OS other than I didn't want to do too many things in the initial patch. I don't think it would be hard to do this on x86-64. For windows, it looks like we'll have to emit the markers and the ARC calls before CodeGen (before WinEHPrepare) is run, but I haven't thought it through.

2336

The check for module flag was here because the initial version of this patch added the attribute when the target wasn't arm64 (e.g., x86-64) too. I've removed the check for now.

compnerd added inline comments.Dec 9 2020, 8:32 AM
clang/lib/CodeGen/CGObjC.cpp
2328

Nit: I think that > 0 is easier to read.

2330

Hmm, why the explicit check for not-Windows?

llvm/include/llvm/Analysis/ObjCARCRVAttr.h
44

Nit: A new line before this would be nice.

llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp
443

It seems that IsBundled is unused after this point, why not just do:

if (BundledInsts->contains(Inst))
  return false;
ahatanak updated this revision to Diff 310561.Dec 9 2020, 9:14 AM
ahatanak marked 3 inline comments as done.

Address review comment.

clang/lib/CodeGen/CGObjC.cpp
2330

My understanding is that we'll have to do things differently in ARC contract on Windows, so for now, this patch disables this optimization on Windows.

Is the FIXME comment about Windows I added correct?.

There is nothing particularly special about that. The reason for the funclet handling there is that in the case of an outlined block for exception handling (i.e. a funclet), we need to ensure that the assembly marker received the funclet token as failure to do so would mark the assembly as unreachable and would thus thwart the auto-release claim. For the normal codepath, the behaviour should be identical.

ahatanak updated this revision to Diff 310568.Dec 9 2020, 9:31 AM

Fix an incorrect comment in insertRetainOrClaimRVCalls in InlineFunction.cpp.

There is nothing particularly special about that. The reason for the funclet handling there is that in the case of an outlined block for exception handling (i.e. a funclet), we need to ensure that the assembly marker received the funclet token as failure to do so would mark the assembly as unreachable and would thus thwart the auto-release claim. For the normal codepath, the behaviour should be identical.

ARC contract pass removes attribute retainRV/claimRV (but not attribute rv_marker) from a call and inserts a retainRV/claimRV call after the call in the IR. In that case, the inserted retainRV/claimRV should get the funclet token. Is that right?

Hmm, it seems that calls to some of the ARC runtime functions don't get the token. In some cases, I'm seeing WinEHPrepare (ToT clang, without this patch) turn a call to @objc_unsafeClaimAutoreleasedReturnValue in an EH cleanup block into an unreachable.

ahatanak updated this revision to Diff 310693.Dec 9 2020, 3:55 PM
ahatanak edited the summary of this revision. (Show Details)

Enable the optimization on Windows.

Move createCallInst defined in ObjCARCContract.cpp to ObjCARC.h and use it to create calls to retainRV/claimRV with funclet tokens when ARC contraction is run. Add a test case to contract-marker-funclet.ll.

ToT clang/llvm (without this patch) is turning the call to @objc_unsafeClaimAutoreleasedReturnValue in the EH cleanup block into an unreachable when compiling the following code. It looks like this happens because the inliner isn't adding the funclet token, but it's not clear to me whether this is a bug or not.

$ cat test.mm

void foo();
id noexcept_func() noexcept;

struct S0 {
  id x;
  ~S0() {
    noexcept_func();
  }
};

void test_attr_claimRV() {
  S0 s0;
  foo();
}

$ clang++ -std=c++11 -fobjc-arc test.mm -O3 -S -o - "-fobjc-runtime=ios-11.0.0" -target aarch64-unknown-windows-msvc

ahatanak updated this revision to Diff 310709.Dec 9 2020, 4:24 PM

Elaborate on when createCallInstWithColors should be called.

Hmm, I think that the function is marked as noexcept, so the the funclet shouldn't be invoked and should get DCE'd, but that seems unrelated to ARC.

ahatanak updated this revision to Diff 311662.Dec 14 2020, 11:42 AM

Set the insert point before generating new instructions in insertRetainOrClaimRVCalls.

ahatanak updated this revision to Diff 315886.Mon, Jan 11, 11:43 AM

Rebase and fix a few bugs in the patch.

  • In the loop in DeadArgumentEliminationPass::SurveyFunction that visits the uses of a function, continue instead of break so that the function is marked live when a function is used by something that isn't a CallBase.
  • Make ObjCARCOpt::OptimizeIndividualCallImpl return early if the instruction is a retainRV/claimRV that was temporarily inserted after a call annotated with a retainRV/claimRV attribute. This fixes the first assertion in ObjCARCOpt::OptimizeRetainRVCall.