[CodeGen] Split out cold exception handling pads.

[snehasish]

Summary

Support for splitting exception handling pads was added in D73739. This
change updates the code to split out exception handling pads if profile
information indicates that they are cold.

[rahmanl]

Wouldn't this cause an error when we have both hot and cold eh pads? We must ensure that same section id is assigned to all landing pads (please refer to BasicBlockSections.cpp::assignSections.

[snehasish]

Extend the check to account of multiple landing pads with difference in hotness.
Add a new test to verify the expected behaviour.

[snehasish]

Yes, this would cause an issue if we had hot and cold landing pads. I've updated the logic to account for this and added a test. PTAL, thanks!

[snehasish]

I tried this patch on an instrumented build of mysql server. With this patch, an additional 28KB of text is split out from hot functions, a small but non-trivial improvement considering the size of the .text.hot section is ~1MB.

[rahmanl]

Thanks.
LGTM with minor nits.

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

138

"Use auto & for values and auto * for pointers unless you need to make a copy."
You might as well consider spelling out the type here since the type is not really obvious from this line.

143

Same as above.

llvm/test/CodeGen/X86/machine-function-splitter.ll

206

nit: You can reuse this for the previous test case (the case where all landing pads are cold). You can still have two landing pads and make one of them configurable with a macro.

hot:
  %2 = call i32 @bar()
  invoke void @_Z1fv()
          to label %exit unwind label %lpad2, !prof ![[PROF]]

Just a suggestion. Using macros could make tests less readable too.

[snehasish]

Spell out types instead of auto.

[snehasish]

Thanks for the review.

llvm/test/CodeGen/X86/machine-function-splitter.ll

206

It's an interesting idea but I had a couple of concerns so I left it as is

• It's really easy to miss the macro since the substitution is for the profile metadata
• We would need to invoke llc again with two version of the macro

[rahmanl]

llvm/test/CodeGen/X86/machine-function-splitter.ll

206

SGTM. Thanks for considering.

llvm/lib/CodeGen/MachineFunctionSplitter.cpp

 // decreases fragmentation and improves icache and itlb utilization. Note that
 // the overall changes to the binary size are negligible; only a small number of
 // additional jump instructions may be introduced.
 //
 // For the original RFC of this pass please see
 // https://groups.google.com/d/msg/llvm-dev/RUegaMg-iqc/wFAVxa6fCgAJ
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/CodeGen/BasicBlockSectionUtils.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
public:
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   bool runOnMachineFunction(MachineFunction &F) override;
 };
 } // end anonymous namespace
 
-static bool isColdBlock(MachineBasicBlock &MBB,
+static bool isColdBlock(const MachineBasicBlock &MBB,
                         const MachineBlockFrequencyInfo *MBFI,
                         ProfileSummaryInfo *PSI) {
   Optional<uint64_t> Count = MBFI->getBlockProfileCount(&MBB);
   if (!Count.hasValue())
     return true;
 
   if (PercentileCutoff > 0) {
     return PSI->isColdCountNthPercentile(PercentileCutoff, *Count);
bool MachineFunctionSplitter::runOnMachineFunction(MachineFunction &MF) {
   // sortBasicBlocksAndUpdateBranches uses the numeric identifier to sort
   // blocks. Preserving the order of blocks is essential to retaining decisions
   // made by prior passes such as MachineBlockPlacement.
   MF.RenumberBlocks();
   MF.setBBSectionsType(BasicBlockSection::Preset);
   auto *MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
   auto *PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
 
+  SmallVector<MachineBasicBlock *, 2> LandingPads;
   for (auto &MBB : MF) {
-    // FIXME: We retain the entry block and conservatively keep all landing pad
-    // blocks as part of the original function. Once D73739 is submitted, we can
-    // improve the handling of ehpads.
-    if ((MBB.pred_empty() || MBB.isEHPad()))
+    if (MBB.isEntryBlock())
       continue;
-    if (isColdBlock(MBB, MBFI, PSI))
+
+    if (MBB.isEHPad())
+      LandingPads.push_back(&MBB);
+    else if (isColdBlock(MBB, MBFI, PSI))
       MBB.setSectionID(MBBSectionID::ColdSectionID);
   }
 
+  // We only split out eh pads if all of them are cold.
+  bool HasHotLandingPads = false;
+  for (const MachineBasicBlock *LP : LandingPads) {
+    if (!isColdBlock(*LP, MBFI, PSI))
+      HasHotLandingPads = true;
+  }
+  if (!HasHotLandingPads) {
+    for (MachineBasicBlock *LP : LandingPads)
+      LP->setSectionID(MBBSectionID::ColdSectionID);
+  }
+
   auto Comparator = [](const MachineBasicBlock &X, const MachineBasicBlock &Y) {
     return X.getSectionID().Type < Y.getSectionID().Type;
   };
   llvm::sortBasicBlocksAndUpdateBranches(MF, Comparator);
 
   return true;
 }
 

llvm/test/CodeGen/X86/machine-function-splitter.ll

4:                                                ; preds = %1
   br label %6
 
 6:                                                ; preds = %4, %2
   %7 = tail call i32 @qux()
   ret void
 }
 
 define i32 @foo7(i1 zeroext %0) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !prof !14 {
-;; Check that cold ehpads are not split out.
+;; Check that a single cold ehpad is split out.
 ; MFS-DEFAULTS-LABEL: foo7
 ; MFS-DEFAULTS:       .section        .text.split.foo7,"ax",@progbits
 ; MFS-DEFAULTS-NEXT:  foo7.cold:
-; MFS-DEFAULTS-NOT:   callq   _Unwind_Resume
 ; MFS-DEFAULTS:       callq   baz
+; MFS-DEFAULTS:       callq   _Unwind_Resume@PLT
 entry:
   invoke void @_Z1fv()
           to label %try.cont unwind label %lpad
 
 lpad:
   %1 = landingpad { i8*, i32 }
           cleanup
           catch i8* bitcast (i8** @_ZTIi to i8*)
4:                                                ; preds = %1
   %5 = call i32 @baz()
   br label %6
 
 6:                                                ; preds = %4, %2
   %7 = tail call i32 @qux()
   ret i32 %7
 }
 
+define i32 @foo8(i1 zeroext %0) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !prof !14 {
+;; Check that all ehpads are treated as hot if one of them is hot.
+; MFS-DEFAULTS-LABEL: foo8
+; MFS-DEFAULTS:       callq   _Unwind_Resume@PLT
+; MFS-DEFAULTS:       callq   _Unwind_Resume@PLT
+; MFS-DEFAULTS:       .section        .text.split.foo8,"ax",@progbits
+; MFS-DEFAULTS-NEXT:  foo8.cold:
+; MFS-DEFAULTS:       callq   baz
+entry:
+  invoke void @_Z1fv()
+          to label %try.cont unwind label %lpad1
+
+lpad1:
+  %1 = landingpad { i8*, i32 }
+          cleanup
+          catch i8* bitcast (i8** @_ZTIi to i8*)
+  resume { i8*, i32 } %1
+
+try.cont:
+  br i1 %0, label %hot, label %cold, !prof !17
+
+hot:
+  %2 = call i32 @bar()
+  invoke void @_Z1fv()
+          to label %exit unwind label %lpad2, !prof !21
+
+lpad2:
+  %3 = landingpad { i8*, i32 }
+          cleanup
+          catch i8* bitcast (i8** @_ZTIi to i8*)
+  resume { i8*, i32 } %3
+
+cold:
+  %4 = call i32 @baz()
+  br label %exit
+
+exit:
+  %5 = tail call i32 @qux()
+  ret i32 %5
+}
+
 declare i32 @bar()
 declare i32 @baz()
 declare i32 @bam()
 declare i32 @qux()
 declare void @_Z1fv()
 declare i32 @__gxx_personality_v0(...)
 
 @_ZTIi = external constant i8*