[ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

[sgraenitz]

Summary

When optimizing retain-release-sequences we insert (and delete) ObjC runtime calls. These calls need a funclet operand bundle that refers to the enclosing funclet pad whenever they are inserted in a WinEH funclet. WinEH funclets can contain multiple basic blocks. In order to find the enclosing funclet pad, we have to calculate the funclet coloring first.

[sgraenitz]

There was a similar effort for CleanupPads in the past: https://reviews.llvm.org/rG8b342680bf62722e5099074e8bd23491c71d92b3

[sgraenitz]

There doesn't seem to be a 1-to-1 relationship between deleted and inserted retain/release calls and thus we don't attempt to preserve bundle operands in general.

[rnk]

I can't say I'm familiar with ARC, but this seems right to me.

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

767

I don't think the verifier changes you made guarantee this. I would consider strengthening this to report_fatal_error, since valid IR transforms can probably reach this code.

2040–2041

I think this code snippet probably deserves a Function helper, F->hasScopedEHPersonality(). Another part of me thinks we should cache the personality enum similar to the way we cache intrinsic ids, but I wouldn't want to increase Function memory usage, so that seems premature. "No action required", I guess.

[sgraenitz]

Thanks for your feedback @rnk! I hope some of the ObjCARCOpts authors will share their opinions as well.

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

767

Right, I guess the Verifier change is correct and this should change to work for multi-color BBs?

assert(CV.size() > 0 && "Uncolored block");
for (BasicBlock *EHPadBB : CV)
  if (auto *EHPad = dyn_cast<FuncletPadInst>(ColorFirstBB->getFirstNonPHI())) {
    OpBundles.emplace_back("funclet", EHPad);
    break;
  }

There is no test that covers this case, but it appears that the unique color invariant only holds after WinEHPrepare. The assertion here seems to imply it:

assert(BBColors.size() == 1 && "multi-color BB not removed by preparation");

Since it would be equivalent to the Verifier check, I'd stick with the assertion and not report a fatal error.

2040–2041

It doesn't really fit the the scope of this patch but I am happy to add the helper function in a follow-up (for now without personality enum caching).

[sgraenitz]

Rebase and update check-lines after D137939

[sgraenitz]

Handle potential multi-color basic blocks in addOpBundleForFunclet()

[sgraenitz]

This is ready to land and I'd appreciate feedback from one of the authors. Anyone else I should add for review?

[ahatanak]

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

767

Is the assertion assert(CV.size() == 1 && "non-unique color for block!"); in CloneCallInstForBB incorrect too?

2040–2041

OptimizeIndividualCall calls colorEHFunclets too. Is it possible to just call it once? Or we don't care because it's not expensive?

[sgraenitz]

Address feedback

[sgraenitz]

Thanks for taking a look. Yes, the coloring can be expensive and we shouldn't run it more than once. OptimizeIndividualCalls did it and actually even OptimizeSequences runs in a loop "until it either stops making changes or no retain+release pair nesting is detected". I completely missed that.

I moved funclet coloring into ObjCARCOpt::init() so that it runs once for the whole pass now. The results are cached in ObjCARCOpt::BlockEHColors. addOpBundleForFunclet() and cloneCallInstForBB became member functions that can directly use the cached results. We don't need to pass them as parameters from function to function anymore. I think it's not only more efficient now but also much cleaner. That was really useful feedback @ahatanak! Thanks

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

767

The code path disappears with the subsequent patch D137945. But yes, it's formally incorrect and I adjusted the assertion in the last iteration.

2040–2041

Good catch! Right, we shouldn't do it multiple times. Having moved funclet coloring into ObjCARCOpt::init() it now runs only once.

[sgraenitz]

Ping

[sgraenitz]

This review belongs to a series of patches. I am planning to land it towards the end of next week, so that it's ready for the next release branching in February. If you have any more remarks, please let me know soon.

[ahatanak]

I'm not familiar with the WinEH stuff, but the other parts (the code that adds bundles, etc.) LGTM.

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

598

This piece of code does something similar to cloneCallInstForBB, but it's slightly different from it. It iterates over the entries in CV whereas the code above just looks at the first entry.

Is this a bug that is fixed in https://reviews.llvm.org/D137945?

[sgraenitz]

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

598

Yes, D137945 unifies the behavior so that we will always iterate. The unique color invariant only holds after WinEHPrepare. So the old assertion in cloneCallInstForBB() was formally wrong, but I only ever observed cases of non-unique coloring in incorrect IR, i.e. dangling funclet tokens before D137939. Iteration is the safe solution, especially since we may not be able to bail out on dangling funclet tokens in the verifier (see https://reviews.llvm.org/D138123#4067201).

FYI: We discussed it this in a previous round of this review https://reviews.llvm.org/D137944?id=475132#inline-1334823

751

The old code here assumed all EH coloring to be unique.

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

 // RUN: %clang_cc1 -O0 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O0
-// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -mllvm -enable-objc-arc-opts=false -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O2
+// RUN: %clang_cc1 -O2 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-O2
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
 // funclet tokens for llvm.objc.* calls inside catchpads and that they refer to
 // their catchpad's SSA value.
 

llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

bool VisitTopDown(
           &ReleaseInsertPtToRCIdentityRoots);
   bool Visit(Function &F, DenseMap<const BasicBlock *, BBState> &BBStates,
              BlotMapVector<Value *, RRInfo> &Retains,
              DenseMap<Value *, RRInfo> &Releases);
 
   void MoveCalls(Value *Arg, RRInfo &RetainsToMove, RRInfo &ReleasesToMove,
                  BlotMapVector<Value *, RRInfo> &Retains,
                  DenseMap<Value *, RRInfo> &Releases,
-                 SmallVectorImpl<Instruction *> &DeadInsts, Module *M);
+                 SmallVectorImpl<Instruction *> &DeadInsts,
+                 const DenseMap<BasicBlock *, ColorVector> &BlockColors,
+                 Module *M);
 
   bool PairUpRetainsAndReleases(DenseMap<const BasicBlock *, BBState> &BBStates,
                                 BlotMapVector<Value *, RRInfo> &Retains,
                                 DenseMap<Value *, RRInfo> &Releases, Module *M,
                                 Instruction *Retain,
                                 SmallVectorImpl<Instruction *> &DeadInsts,
                                 RRInfo &RetainsToMove, RRInfo &ReleasesToMove,
                                 Value *Arg, bool KnownSafe,
                                 bool &AnyPairsCompletelyEliminated);
 
   bool PerformCodePlacement(DenseMap<const BasicBlock *, BBState> &BBStates,
                             BlotMapVector<Value *, RRInfo> &Retains,
-                            DenseMap<Value *, RRInfo> &Releases, Module *M);
+                            DenseMap<Value *, RRInfo> &Releases, Function &F);
 
   void OptimizeWeakCalls(Function &F);
 
   bool OptimizeSequences(Function &F);
 
   void OptimizeReturns(Function &F);
 
 #ifndef NDEBUG
if (const Instruction *Call = dyn_cast<CallBase>(Arg)) {
     if (Call->getParent() == RetainRV->getParent()) {
       BasicBlock::const_iterator I(Call);
       ++I;
       while (IsNoopInstruction(&*I))
         ++I;
       if (&*I == RetainRV)
         return false;
     } else if (const InvokeInst *II = dyn_cast<InvokeInst>(Call)) {
       BasicBlock *RetainRVParent = RetainRV->getParent();
       if (II->getNormalDest() == RetainRVParent) {
         BasicBlock::const_iterator I = RetainRVParent->begin();
         while (IsNoopInstruction(&*I))
           ++I;
         if (&*I == RetainRV)
           return false;
       }
     }
for (unsigned I = 0, E = CI.getNumOperandBundles(); I != E; ++I) {
     // Funclets will be reassociated in the future.
     if (Bundle.getTagID() == LLVMContext::OB_funclet)
       continue;
     OpBundles.emplace_back(Bundle);
   }
 
   if (!BlockColors.empty()) {
     const ColorVector &CV = BlockColors.find(&BB)->second;
     assert(CV.size() == 1 && "non-unique color for block!");
     Instruction *EHPad = CV.front()->getFirstNonPHI();
     if (EHPad->isEHPad())
       OpBundles.emplace_back("funclet", EHPad);
   }
 
   return CallInst::Create(&CI, OpBundles);
 }
+
+void addOpBundleForFunclet(BasicBlock *BB,
+                           const DenseMap<BasicBlock *, ColorVector> &BlockColors,
+                           SmallVectorImpl<OperandBundleDef> &OpBundles) {
+  if (!BlockColors.empty()) {
+    const ColorVector &CV = BlockColors.find(BB)->second;
+    assert(CV.size() == 1 && "non-unique color for block!");
+    BasicBlock *EHPadBB = CV.front();
+    if (auto *EHPad = dyn_cast<FuncletPadInst>(EHPadBB->getFirstNonPHI()))
+      OpBundles.emplace_back("funclet", EHPad);
+  }
+}
 }
 
 /// Visit each call, one at a time, and make simplifications without doing any
 /// additional analysis.
 void ObjCARCOpt::OptimizeIndividualCalls(Function &F) {
   LLVM_DEBUG(dbgs() << "\n== ObjCARCOpt::OptimizeIndividualCalls ==\n");
   // Reset all the flags in preparation for recomputing them.
   UsedInThisFunction = 0;
for (BasicBlock *BB : llvm::reverse(PostOrder)) {
     if (DisableRetainReleasePairing)
       return false;
   }
 
   return TopDownNestingDetected && BottomUpNestingDetected;
 }
 
 /// Move the calls in RetainsToMove and ReleasesToMove.
-void ObjCARCOpt::MoveCalls(Value *Arg, RRInfo &RetainsToMove,
-                           RRInfo &ReleasesToMove,
-                           BlotMapVector<Value *, RRInfo> &Retains,
-                           DenseMap<Value *, RRInfo> &Releases,
-                           SmallVectorImpl<Instruction *> &DeadInsts,
-                           Module *M) {
+void ObjCARCOpt::MoveCalls(
+    Value *Arg, RRInfo &RetainsToMove, RRInfo &ReleasesToMove,
+    const DenseMap<BasicBlock *, ColorVector> &BlockColors, Module *M) {
   Type *ArgTy = Arg->getType();
   Type *ParamTy = PointerType::getUnqual(Type::getInt8Ty(ArgTy->getContext()));
 
   LLVM_DEBUG(dbgs() << "== ObjCARCOpt::MoveCalls ==\n");
 
   // Insert the new retain and release calls.
   for (Instruction *InsertPt : ReleasesToMove.ReverseInsertPts) {
     Value *MyArg = ArgTy == ParamTy ? Arg :
                    new BitCastInst(Arg, ParamTy, "", InsertPt);
     Function *Decl = EP.get(ARCRuntimeEntryPointKind::Retain);
-    CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+    SmallVector<OperandBundleDef, 1> BundleList;
+    addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+    CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
     Call->setDoesNotThrow();
     Call->setTailCall();
 
     LLVM_DEBUG(dbgs() << "Inserting new Retain: " << *Call
                       << "\n"
                          "At insertion point: "
                       << *InsertPt << "\n");
   }
   for (Instruction *InsertPt : RetainsToMove.ReverseInsertPts) {
     Value *MyArg = ArgTy == ParamTy ? Arg :
                    new BitCastInst(Arg, ParamTy, "", InsertPt);
     Function *Decl = EP.get(ARCRuntimeEntryPointKind::Release);
-    CallInst *Call = CallInst::Create(Decl, MyArg, "", InsertPt);
+    SmallVector<OperandBundleDef, 1> BundleList;
+    addOpBundleForFunclet(InsertPt->getParent(), BlockColors, BundleList);
+    CallInst *Call = CallInst::Create(Decl, MyArg, BundleList, "", InsertPt);
     // Attach a clang.imprecise_release metadata tag, if appropriate.
     if (MDNode *M = ReleasesToMove.ReleaseMetadata)
       Call->setMetadata(MDKindCache.get(ARCMDKindID::ImpreciseRelease), M);
     Call->setDoesNotThrow();
     if (ReleasesToMove.IsTailCallRelease)
       Call->setTailCall();
 
     LLVM_DEBUG(dbgs() << "Inserting new Release: " << *Call
bool ObjCARCOpt::PairUpRetainsAndReleases(
   return true;
 }
 
 /// Identify pairings between the retains and releases, and delete and/or move
 /// them.
 bool ObjCARCOpt::PerformCodePlacement(
     DenseMap<const BasicBlock *, BBState> &BBStates,
     BlotMapVector<Value *, RRInfo> &Retains,
-    DenseMap<Value *, RRInfo> &Releases, Module *M) {
+    DenseMap<Value *, RRInfo> &Releases, Function &F) {
   LLVM_DEBUG(dbgs() << "\n== ObjCARCOpt::PerformCodePlacement ==\n");
 
+  DenseMap<BasicBlock *, ColorVector> BlockColors;
+  if (F.hasPersonalityFn() &&
+      isScopedEHPersonality(classifyEHPersonality(F.getPersonalityFn())))
+    BlockColors = colorEHFunclets(F);
+
   bool AnyPairsCompletelyEliminated = false;
   SmallVector<Instruction *, 8> DeadInsts;
 
   // Visit each retain.
   for (BlotMapVector<Value *, RRInfo>::const_iterator I = Retains.begin(),
                                                       E = Retains.end();
        I != E; ++I) {
     Value *V = I->first;
if (const LoadInst *LI = dyn_cast<LoadInst>(Arg))
         if (GV->isConstant())
           KnownSafe = true;
 
     // Connect the dots between the top-down-collected RetainsToMove and
     // bottom-up-collected ReleasesToMove to form sets of related calls.
     RRInfo RetainsToMove, ReleasesToMove;
 
     bool PerformMoveCalls = PairUpRetainsAndReleases(
-        BBStates, Retains, Releases, M, Retain, DeadInsts,
+        BBStates, Retains, Releases, F.getParent(), Retain, DeadInsts,
         RetainsToMove, ReleasesToMove, Arg, KnownSafe,
         AnyPairsCompletelyEliminated);
 
     if (PerformMoveCalls) {
       // Ok, everything checks out and we're all set. Let's move/delete some
       // code!
-      MoveCalls(Arg, RetainsToMove, ReleasesToMove,
-                Retains, Releases, DeadInsts, M);
+      MoveCalls(Arg, RetainsToMove, ReleasesToMove, Retains, Releases,
+                DeadInsts, BlockColors, F.getParent());
     }
   }
 
   // Now that we're done moving everything, we can delete the newly dead
   // instructions, as we no longer need them as insert points.
   while (!DeadInsts.empty())
     EraseInstruction(DeadInsts.pop_back_val());
 
bool ObjCARCOpt::OptimizeSequences(Function &F) {
 
   // Analyze the CFG of the function, and all instructions.
   bool NestingDetected = Visit(F, BBStates, Retains, Releases);
 
   if (DisableRetainReleasePairing)
     return false;
 
   // Transform.
-  bool AnyPairsCompletelyEliminated = PerformCodePlacement(BBStates, Retains,
-                                                           Releases,
-                                                           F.getParent());
+  bool AnyPairsCompletelyEliminated =
+      PerformCodePlacement(BBStates, Retains, Releases, F);
 
   return AnyPairsCompletelyEliminated && NestingDetected;
 }
 
 /// Check if there is a dependent call earlier that does not have anything in
 /// between the Retain and the call that can affect the reference count of their
 /// shared pointer argument. Note that Retain need not be in BB.
 static CallInst *HasSafePathToPredecessorCall(const Value *Arg,

llvm/test/Transforms/ObjCARC/funclet-catchpad.ll

+; RUN: opt -mtriple=x86_64-windows-msvc -passes=objc-arc -S < %s | FileCheck %s
+
+; Check that funclet tokens are preserved
+;
+; CHECK-LABEL:  catch:
+; CHECK:          %1 = catchpad within %0
+; CHECK:          %2 = tail call ptr @llvm.objc.retain(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:          call void @llvm.objc.release(ptr %exn) #0 [ "funclet"(token %1) ]
+; CHECK:          catchret from %1 to label %eh.cont
+
+define void @try_catch_with_objc_intrinsic() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  invoke void @may_throw(ptr null) to label %eh.cont unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+eh.cont:                                          ; preds = %catch, %entry
+  ret void
+
+catch:                                            ; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr %exn.slot]
+  br label %if.then
+
+if.then:                                          ; preds = %catch
+  %exn = load ptr, ptr null, align 8
+  %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
+  call void @may_throw(ptr %exn)
+  call void @llvm.objc.release(ptr %exn) [ "funclet"(token %1) ]
+  catchret from %1 to label %eh.cont
+}
+
+declare void @may_throw(ptr)
+declare i32 @__CxxFrameHandler3(...)
+
+declare ptr @llvm.objc.retain(ptr) #0
+declare void @llvm.objc.release(ptr) #0
+
+attributes #0 = { nounwind }