[PATCH] D104953: [ObjC][ARC] Prevent moving objc_retain calls past objc_release calls that release the retained object

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM, thanks! It might be good to also add a test case where the insert points are not in the same basic block, ie there’s some control flow between them.

I ’m not super familiar with the code, but @ahatanak was kind enough to walk me through the details offline. The approach looks like a conservative approach to address this miscompile and less risky than trying to undo the bad movements.

I still think undoing the bad movements might be an interesting direction in the future, because it would improve codegen for the examples in the tests.



================
Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:1501
 
-bool
-ObjCARCOpt::VisitInstructionTopDown(Instruction *Inst,
-                                    DenseMap<Value *, RRInfo> &Releases,
-                                    BBState &MyStates) {
+// Fill ReleaseInsertPtToRCIdentityRoots.
+static void collectReleaseInsertPts(
----------------
Might be helpful to document how it is filled?


================
Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:1517
+
+// Get the RC identity root from an insertion point of an objc_release call.
+// Return nullptr if the passed instruction isn't an insertion point.
----------------
Identity root*s*?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104953