Page MenuHomePhabricator

llvm/ObjCARC: Eliminate inlined AutoreleaseRV calls
ClosedPublic

Authored by dexonsmith on Nov 17 2019, 7:59 PM.

Details

Summary

Pair up inlined AutoreleaseRV calls with their matching RetainRV or
ClaimRV.

  • For RetainRV, delete both instructions.
  • For ClaimRV, replace with a normal Release and then run the usual optimizations.

This avoids problems where more aggressive inlining triggers memory
regressions.

Diff Detail

Event Timeline

dexonsmith created this revision.Nov 17 2019, 7:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2019, 7:59 PM
rjmccall added inline comments.Nov 17 2019, 9:47 PM
llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
681

"into a normal Release"

Please add something like assert(Class == ARCInstKind::ClaimRV); to make it clearer in the code why this is reasonable behavior.

llvm/test/Transforms/ObjCARC/inlined-autorelease-return-value.ll
115

Do you want to add a test for when there's any sort of interference besides just being in separate blocks? Or for when there are bitcasts?

Added an assertion for clarity and fixed a comment.

dexonsmith marked 3 inline comments as done.Nov 17 2019, 10:11 PM
dexonsmith added inline comments.
llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
681

Fixed in the updated diff.

llvm/test/Transforms/ObjCARC/inlined-autorelease-return-value.ll
115

Yes, I'll add both of those. For interference, is it enough to put a call to llvm.objc.retain in between, or is there something else worth testing?

rjmccall added inline comments.Nov 18 2019, 8:42 AM
llvm/test/Transforms/ObjCARC/inlined-autorelease-return-value.ll
115

Maybe also an opaque call.

dexonsmith marked an inline comment as done.

Adding more testcases. The bitcast tests uncovered problems with the RAUW calls, which I also fixed.

dexonsmith marked 3 inline comments as done.Nov 18 2019, 11:22 AM
dexonsmith added inline comments.
llvm/test/Transforms/ObjCARC/inlined-autorelease-return-value.ll
115

Please see elide_with_{retain,claim}RV_{bitcast,splitBy{Retain,Opaque}}.

dexonsmith marked an inline comment as done.Nov 18 2019, 11:26 AM

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

Respond to Akira's feedback.

  • Use isSafeBetweenRVCalls instead of skipping past all opaque calls.
  • Delete the other user of isSafeBetweenRVCalls in ObjCARCOpt::OptimizeRetainRVCall, since the new code already covers those cases.
  • Check for PHI node equivalence.

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

I thought it was okay to skip over all opaque calls, but I've changed the code to check for this.

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

I thought it was okay to skip over all opaque calls, but I've changed the code to check for this.

I talked to @ahatanak in person and I believe we should compromise. I feel confident that skipping over non-ARC intrinsics and non-call/invoke instructions is safe. Skipping those is sufficient to skip any function teardown logic that the inliner could leave behind. Importantly, this will also catch intrinsics that get added to function epilogues in the future, without having to update a static list in the ARC optimizer.

While I suspect that skipping opaque function calls is also sound, I'm not as confident and it's not relevant in practice.

@ahatanak, you were going to keep thinking about this. Please confirm once you've had a chance to think it through.

@rjmccall, WDYT?

I think I agree with you. isSafeBetweenRVCalls seems too conservative. I can't think of a case where we aren't allowed to remove the pair of instructions because the instructions you listed exist between the pair (and that includes opaque calls). But John probably knows better whether that is safe or not.

The existence of *any* significant intervening code here indicates that either the optimizer moved it there (which is problematic but unfortunately possible with this representation) or that the caller+callee weren't actually paired and the optimization wouldn't have succeeded dynamically. Now, the ARC spec gives us some flexibility to try to avoid the autorelease anyway in the latter case; that does not actually have to be conditioned on seeing a retainARV/unsafeClaimARV, because ARC just says that it will ensure that the value is still valid when the caller gets it and that the caller shouldn't assume that the value is in the autorelease pool. But it's definitely odder territory.

dexonsmith accepted this revision.Nov 19 2019, 12:03 PM

@rjmccall gave me an LGTM offline.

This revision is now accepted and ready to land.Nov 19 2019, 12:03 PM