Assign correct edge weights to unwind destinations when lowering invoke statement.

[congh]

Summary

When lowering invoke statement, all unwind destinations are directly added as successors of call site block, and the weight of those new edges are not assigned properly. Actually, default weight 16 are used for those edges. This patch calculates the proper edge weights for those edges when collecting all unwind destinations.

[hans]

I'm not familiar with this code. Reid is probably a better reviewer here.

[congh]

In D13354#258017, @hans wrote:

I'm not familiar with this code. Reid is probably a better reviewer here.

Thanks for the reviewer recommendation, Hans!

Reid, could you please take a look at this patch?

[congh]

Ping?

[rnk]

Sorry, I missed this the first time around. Looking...

[rnk]

This seems like the right approach. My first thought was to add logic similar to findUnwindDestinations in BPI::getEdgeWeight, but after thinking more I like this is better.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1203

This is just code that runs on every iteration of the while loop. We don't need a lambda, just put it before the end of the loop. You'll have to create a NewEHPadBB local variable and assign it to EHPadBB, but that seems OK to me.

test/CodeGen/X86/catchpad-weight.ll

1

I think the IR for this C++ code would make a better test case:

struct A {};
struct B {};
struct C {};
struct HasDtor {
  ~HasDtor();
};
void may_throw();
int main() {
  HasDtor o;
  try {
    may_throw();
  } catch (A) {
  } catch (B) {
  } catch (C) {
  }
}

Then the invoke for may_throw has five total successors: the normal edge, the three catchpads, and the cleanup.

You can get the catchpad IR like this:

$ clang -c --target=x86_64-windows-msvc t.cpp -S -emit-llvm -o - -fexceptions -O2

[congh]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1203

I have removed the lambda by putting it to the end of the loop (the else case should escape from this part).

test/CodeGen/X86/catchpad-weight.ll

1

Yes, this test case is more complete. I have replaced the old test case with this one. Thanks!

[congh]

Update the patch according to Reid's comment. Adjust some test cases after merging with the up-to-date trunk version.

[rnk]

lgtm

Correcting the weights seems to have fixed the strange, non-source order block placement. Thanks!

[congh]

Thanks for the review, Reid!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 #include "llvm/Target/TargetFrameLowering.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetIntrinsicInfo.h"
 #include "llvm/Target/TargetLowering.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Target/TargetSelectionDAGInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 #include <algorithm>
+#include <utility>
 using namespace llvm;
 
 #define DEBUG_TYPE "isel"
 
 /// LimitFloatPrecision - Generate low-precision inline sequences for
 /// some float libcalls (6, 8 or 12 bits).
 static unsigned LimitFloatPrecision;
 
void SelectionDAGBuilder::visitCleanupPad(const CleanupPadInst &CPI) {
   FuncInfo.MBB->setIsEHFuncletEntry();
 }
 
 /// When an invoke or a cleanupret unwinds to the next EH pad, there are
 /// many places it could ultimately go. In the IR, we have a single unwind
 /// destination, but in the machine CFG, we enumerate all the possible blocks.
 /// This function skips over imaginary basic blocks that hold catchpad,
 /// terminatepad, or catchendpad instructions, and finds all the "real" machine
-/// basic block destinations.
-static void
-findUnwindDestinations(FunctionLoweringInfo &FuncInfo,
-                       const BasicBlock *EHPadBB,
-                       SmallVectorImpl<MachineBasicBlock *> &UnwindDests) {
+/// basic block destinations. As those destinations may not be successors of
+/// EHPadBB, here we also calculate the edge weight to those destinations. The
+/// passed-in Weight is the edge weight to EHPadBB.
+static void findUnwindDestinations(
+    FunctionLoweringInfo &FuncInfo, const BasicBlock *EHPadBB, uint32_t Weight,
+    SmallVectorImpl<std::pair<MachineBasicBlock *, uint32_t>> &UnwindDests) {
   bool IsMSVCCXX = classifyEHPersonality(FuncInfo.Fn->getPersonalityFn()) ==
                    EHPersonality::MSVC_CXX;
+
+  BranchProbabilityInfo *BPI = FuncInfo.BPI;
+  // A lambda that updates the weight of the edge to the new EHPadBB.
+  auto GetUnwindDest = [BPI, &Weight](const BasicBlock *EHPadBB,
+                                      const BasicBlock *NewEHPadBB) {
+    if (BPI && NewEHPadBB)
+      Weight = BPI->getEdgeProbability(EHPadBB, NewEHPadBB).scale(Weight);
+    return NewEHPadBB;
+  };
+
   while (EHPadBB) {
     const Instruction *Pad = EHPadBB->getFirstNonPHI();
     if (isa<LandingPadInst>(Pad)) {
       // Stop on landingpads. They are not funclets.
-      UnwindDests.push_back(FuncInfo.MBBMap[EHPadBB]);
+      UnwindDests.emplace_back(FuncInfo.MBBMap[EHPadBB], Weight);
       break;
     } else if (isa<CleanupPadInst>(Pad) || isa<LandingPadInst>(Pad)) {
       // Stop on cleanup pads. Cleanups are always funclet entries for all known
       // personalities.
-      UnwindDests.push_back(FuncInfo.MBBMap[EHPadBB]);
-      UnwindDests.back()->setIsEHFuncletEntry();
+      UnwindDests.emplace_back(FuncInfo.MBBMap[EHPadBB], Weight);
+      UnwindDests.back().first->setIsEHFuncletEntry();
       break;
     } else if (const auto *CPI = dyn_cast<CatchPadInst>(Pad)) {
       // Add the catchpad handler to the possible destinations.
-      UnwindDests.push_back(FuncInfo.MBBMap[CPI->getNormalDest()]);
+      UnwindDests.emplace_back(FuncInfo.MBBMap[CPI->getNormalDest()], Weight);
       // In MSVC C++, catchblocks are funclets and need prologues.
       if (IsMSVCCXX)
-        UnwindDests.back()->setIsEHFuncletEntry();
-      EHPadBB = CPI->getUnwindDest();
+        UnwindDests.back().first->setIsEHFuncletEntry();
+      EHPadBB = GetUnwindDest(EHPadBB, CPI->getUnwindDest());
     } else if (const auto *CEPI = dyn_cast<CatchEndPadInst>(Pad)) {
-      EHPadBB = CEPI->getUnwindDest();
+      EHPadBB = GetUnwindDest(EHPadBB, CEPI->getUnwindDest());
     } else if (const auto *CEPI = dyn_cast<CleanupEndPadInst>(Pad)) {
-      EHPadBB = CEPI->getUnwindDest();
+      EHPadBB = GetUnwindDest(EHPadBB, CEPI->getUnwindDest());
     }
   }
 }
 
 void SelectionDAGBuilder::visitCleanupRet(const CleanupReturnInst &I) {
   // Update successor info.
-  // FIXME: The weights for catchpads will be wrong.
-  SmallVector<MachineBasicBlock *, 1> UnwindDests;
-  findUnwindDestinations(FuncInfo, I.getUnwindDest(), UnwindDests);
-  for (MachineBasicBlock *UnwindDest : UnwindDests) {
-    UnwindDest->setIsEHPad();
-    addSuccessorWithWeight(FuncInfo.MBB, UnwindDest);
+  SmallVector<std::pair<MachineBasicBlock *, uint32_t>, 1> UnwindDests;
+  auto UnwindDest = I.getUnwindDest();
+  BranchProbabilityInfo *BPI = FuncInfo.BPI;
+  uint32_t UnwindDestWeight =
+      BPI ? BPI->getEdgeWeight(FuncInfo.MBB->getBasicBlock(), UnwindDest) : 0;
+  findUnwindDestinations(FuncInfo, UnwindDest, UnwindDestWeight, UnwindDests);
+  for (auto &UnwindDest : UnwindDests) {
+    UnwindDest.first->setIsEHPad();
+    addSuccessorWithWeight(FuncInfo.MBB, UnwindDest.first, UnwindDest.second);
   }
 
   // Create the terminator node.
   SDValue Ret =
       DAG.getNode(ISD::CLEANUPRET, getCurSDLoc(), MVT::Other, getControlRoot());
   DAG.setRoot(Ret);
 }
 
void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) {
   // If the value of the invoke is used outside of its defining block, make it
   // available as a virtual register.
   // We already took care of the exported value for the statepoint instruction
   // during call to the LowerStatepoint.
   if (!isStatepoint(I)) {
     CopyToExportRegsIfNeeded(&I);
   }
 
-  SmallVector<MachineBasicBlock *, 1> UnwindDests;
-  findUnwindDestinations(FuncInfo, EHPadBB, UnwindDests);
+  SmallVector<std::pair<MachineBasicBlock *, uint32_t>, 1> UnwindDests;
+  BranchProbabilityInfo *BPI = FuncInfo.BPI;
+  uint32_t EHPadBBWeight =
+      BPI ? BPI->getEdgeWeight(InvokeMBB->getBasicBlock(), EHPadBB) : 0;
+  findUnwindDestinations(FuncInfo, EHPadBB, EHPadBBWeight, UnwindDests);
 
   // Update successor info.
-  // FIXME: The weights for catchpads will be wrong.
   addSuccessorWithWeight(InvokeMBB, Return);
-  for (MachineBasicBlock *UnwindDest : UnwindDests) {
-    UnwindDest->setIsEHPad();
-    addSuccessorWithWeight(InvokeMBB, UnwindDest);
+  for (auto &UnwindDest : UnwindDests) {
+    UnwindDest.first->setIsEHPad();
+    addSuccessorWithWeight(InvokeMBB, UnwindDest.first, UnwindDest.second);
   }
 
   // Drop into normal successor.
   DAG.setRoot(DAG.getNode(ISD::BR, getCurSDLoc(),
                           MVT::Other, getControlRoot(),
                           DAG.getBasicBlock(Return)));
 }
 

test/CodeGen/X86/catchpad-weight.ll

+; RUN: llc -march=x86-64 -print-machineinstrs=expand-isel-pseudos %s -o /dev/null 2>&1 | FileCheck %s
+
+; Check if the edge weight to the catchpad is calculated correctly.
+
+; CHECK: Successors according to CFG: BB#3(1048575) BB#1(1)
+
+define void @try_except() #0 personality i8* bitcast (i32 (...)* @_except_handler3 to i8*) {
+entry:
+  %__exception_code = alloca i32, align 4
+  call void (...) @llvm.localescape(i32* %__exception_code)
+  invoke void @f(i32 1) #3
+          to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %entry
+  %0 = catchpad [i8* bitcast (i32 ()* @try_except_filter_catchall to i8*)] to label %__except.ret unwind label %catchendblock
+
+__except.ret:                                     ; preds = %catch.dispatch
+  catchret %0 to label %__except
+
+__except:                                         ; preds = %__except.ret
+  call void @f(i32 2)
+  br label %__try.cont
+
+__try.cont:                                       ; preds = %__except, %invoke.cont
+  call void @f(i32 3)
+  ret void
+
+catchendblock:                                    ; preds = %catch.dispatch
+  catchendpad unwind to caller
+
+invoke.cont:                                      ; preds = %entry
+  br label %__try.cont
+}
+
+define internal i32 @try_except_filter_catchall() #0 {
+entry:
+  %0 = call i8* @llvm.frameaddress(i32 1)
+  %1 = call i8* @llvm.x86.seh.recoverfp(i8* bitcast (void ()* @try_except to i8*), i8* %0)
+  %2 = call i8* @llvm.localrecover(i8* bitcast (void ()* @try_except to i8*), i8* %1, i32 0)
+  %__exception_code = bitcast i8* %2 to i32*
+  %3 = getelementptr inbounds i8, i8* %0, i32 -20
+  %4 = bitcast i8* %3 to i8**
+  %5 = load i8*, i8** %4, align 4
+  %6 = bitcast i8* %5 to { i32*, i8* }*
+  %7 = getelementptr inbounds { i32*, i8* }, { i32*, i8* }* %6, i32 0, i32 0
+  %8 = load i32*, i32** %7, align 4
+  %9 = load i32, i32* %8, align 4
+  store i32 %9, i32* %__exception_code, align 4
+  ret i32 1
+}
+
+; Function Attrs: nounwind readnone
+declare i8* @llvm.frameaddress(i32) #1
+
+; Function Attrs: nounwind readnone
+declare i8* @llvm.x86.seh.recoverfp(i8*, i8*) #1
+
+; Function Attrs: nounwind readnone
+declare i8* @llvm.localrecover(i8*, i8*, i32) #1
+
+declare void @f(i32) #0
+
+; Function Attrs: nounwind
+declare void @llvm.localescape(...) #2
+
+declare i32 @_except_handler3(...)
+
+attributes #0 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind }
+attributes #3 = { noinline }