[PATCH] D46482: [ObjCARC] Prevent code motion into a catchswitch

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai created this revision.
smeenai added reviewers: ahatanak, rjmccall.

A catchswitch must be the only non-phi instruction in its basic block;
attempting to move a retain or release into a catchswitch basic block
will result in invalid IR. Explicitly mark a CFG hazard in this case to
prevent the code motion.

I'll be honest; this feels like a bit of a hack, and I'm open to other
ideas. I'm putting this up primarily to demonstrate the issue and get
feedback on the approach. I don't think the idea of preventing motion
into a catchswitch is itself bad; the only alternative would be
splitting the edge and inserting a cleanuppad to hold the inserted call,
but I think that would be much more trouble than the optimization is
worth. Rather, I think this might not be the best place to add the check
for the catchswitch. I also tried setting CFGHazardAfflicted inside
BottomUpPtrState::HandlePotentialUse, where the catchswitch is actually
added as an insertion point, but that would set CFGHazardAfflicted on a
retain's RRInfo, whereas the check for CFG hazards preventing code
motion only seems to take the release's RRInfo into account.

Fixes PR37332.


Repository:
  rL LLVM

https://reviews.llvm.org/D46482

Files:
  lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  test/Transforms/ObjCARC/opt-catchswitch.ll


Index: test/Transforms/ObjCARC/opt-catchswitch.ll
===================================================================
--- /dev/null
+++ test/Transforms/ObjCARC/opt-catchswitch.ll
@@ -0,0 +1,54 @@
+; RUN: opt -S -objc-arc < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686--windows-msvc"
+
+declare i8* @f(i8*, i8*)
+
+declare i32 @__CxxFrameHandler3(...)
+
+declare dllimport i8* @objc_autoreleaseReturnValue(i8* returned)
+declare dllimport i8* @objc_retain(i8* returned)
+declare dllimport i8* @objc_retainAutoreleasedReturnValue(i8* returned)
+declare dllimport void @objc_release(i8*)
+
+define i8* @g(i8* %p, i8* %q) local_unnamed_addr personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %0 = tail call i8* @objc_retain(i8* %p) #0
+  %1 = tail call i8* @objc_retain(i8* %q) #0
+  %call = invoke i8* @f(i8* %p, i8* %q)
+          to label %invoke.cont unwind label %catch.dispatch, !clang.arc.no_objc_arc_exceptions !0
+
+catch.dispatch:
+  %2 = catchswitch within none [label %catch] unwind to caller
+
+catch:
+  %3 = catchpad within %2 [i8* null, i32 64, i8* null]
+  catchret from %3 to label %cleanup
+
+invoke.cont:
+  %4 = tail call i8* @objc_retainAutoreleasedReturnValue(i8* %call) #0
+  br label %cleanup
+
+cleanup:
+  %retval.0 = phi i8* [ %call, %invoke.cont ], [ null, %catch ]
+  tail call void @objc_release(i8* %q) #0, !clang.imprecise_release !0
+  tail call void @objc_release(i8* %p) #0, !clang.imprecise_release !0
+  %5 = tail call i8* @objc_autoreleaseReturnValue(i8* %retval.0) #0
+  ret i8* %retval.0
+}
+
+; CHECK-LABEL: entry:
+; CHECK-NEXT:    %0 = tail call i8* @objc_retain(i8* %p) #0
+; CHECK-NEXT:    %call = invoke i8* @f(i8* %p, i8* %q)
+; CHECK-NEXT:            to label %invoke.cont unwind label %catch.dispatch
+
+; CHECK-LABEL: catch.dispatch:
+; CHECK-NEXT:    %1 = catchswitch within none [label %catch] unwind to caller
+
+; CHECK-LABEL: cleanup:
+; CHECK:         tail call void @objc_release(i8* %p) #0
+
+attributes #0 = { nounwind }
+
+!0 = !{}
Index: lib/Transforms/ObjCARC/ObjCARCOpts.cpp
===================================================================
--- lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -1690,6 +1690,12 @@
                        "OverflowOccurredValue.");
                 NewDelta += PathCount;
                 NewCount += PathCount;
+
+                // A catchswitch must be the only non-phi instruction in its
+                // basic block, so attempting to insert an instruction into such
+                // a block would produce invalid IR. Mark it as a CFG hazard to
+                // prevent code motion in this case.
+                CFGHazardAfflicted |= isa<CatchSwitchInst>(RIP);
               }
             }
           NewRetains.push_back(NewReleaseRetain);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D46482.145323.patch
Type: text/x-patch
Size: 2884 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180504/68147b75/attachment.bin>