Merge commit '246b8eae20564e2c34690803efdbaa9cec7c90a1' into cast_cmp

Author: ecnelises

.github/workflows/libcxx-build-and-test.yaml

@@ -22,6 +22,12 @@ on:
       - 'runtimes/**'
       - 'cmake/**'
       - '.github/workflows/libcxx-build-and-test.yaml'
+  schedule:
+    # Run nightly at 8 AM UTC (or roughly 3 AM eastern)
+    - cron: '0 3 * * *'
+
+permissions:
+  contents: read # Default everything to read-only
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
@@ -157,7 +163,11 @@ jobs:
           'generic-no-unicode',
           'generic-no-wide-characters',
           'generic-static',
-          'generic-with_llvm_unwinder'
+          'generic-with_llvm_unwinder',
+          # TODO Find a better place for the benchmark and bootstrapping builds to live. They're either very expensive
+          # or don't provide much value since the benchmark run results are too noise on the bots.
+          'benchmarks',
+          'bootstrapping-build'
         ]
         machine: [ 'libcxx-runners-8' ]
         std_modules: [ 'OFF' ]

.github/workflows/sync-release-repo.yml

@@ -611,6 +611,9 @@ class BinaryContext {
   /// Indicates if the binary contains split functions.
   bool HasSplitFunctions{false};
 
+  /// Indicates if the function ordering of the binary is finalized.
+  bool HasFinalizedFunctionOrder{false};
+
   /// Is the binary always loaded at a fixed address. Shared objects and
   /// position-independent executables (PIEs) are examples of binaries that
   /// will have HasFixedLoadAddress set to false.

bolt/include/bolt/Core/BinaryContext.h

@@ -23,6 +23,9 @@ enum SplitFunctionsStrategy : char {
   /// Split each function into a hot and cold fragment using profiling
   /// information.
   Profile2 = 0,
+  /// Split each function into a hot, warm, and cold fragment using
+  /// profiling information.
+  CDSplit,
   /// Split each function into a hot and cold fragment at a randomly chosen
   /// split point (ignoring any available profiling information).
   Random2,
@@ -40,7 +43,7 @@ class SplitStrategy {
 
   virtual ~SplitStrategy() = default;
   virtual bool canSplit(const BinaryFunction &BF) = 0;
-  virtual bool keepEmpty() = 0;
+  virtual bool compactFragments() = 0;
   virtual void fragment(const BlockIt Start, const BlockIt End) = 0;
 };
 

bolt/include/bolt/Passes/SplitFunctions.h

@@ -3296,7 +3296,7 @@ void BinaryFunction::fixBranches() {
       BB->eraseInstruction(BB->findInstruction(UncondBranch));
 
     // Basic block that follows the current one in the final layout.
-    const BinaryBasicBlock *NextBB =
+    const BinaryBasicBlock *const NextBB =
         Layout.getBasicBlockAfter(BB, /*IgnoreSplits=*/false);
 
     if (BB->succ_size() == 1) {
@@ -3313,39 +3313,54 @@ void BinaryFunction::fixBranches() {
       assert(CondBranch && "conditional branch expected");
       const BinaryBasicBlock *TSuccessor = BB->getConditionalSuccessor(true);
       const BinaryBasicBlock *FSuccessor = BB->getConditionalSuccessor(false);
-      // Check whether we support reversing this branch direction
-      const bool IsSupported = !MIB->isUnsupportedBranch(*CondBranch);
-      if (NextBB && NextBB == TSuccessor && IsSupported) {
+
+      // Eliminate unnecessary conditional branch.
+      if (TSuccessor == FSuccessor) {
+        BB->removeDuplicateConditionalSuccessor(CondBranch);
+        if (TSuccessor != NextBB)
+          BB->addBranchInstruction(TSuccessor);
+        continue;
+      }
+
+      // Reverse branch condition and swap successors.
+      auto swapSuccessors = [&]() {
+        if (MIB->isUnsupportedBranch(*CondBranch))
+          return false;
         std::swap(TSuccessor, FSuccessor);
-        {
-          auto L = BC.scopeLock();
-          MIB->reverseBranchCondition(*CondBranch, TSuccessor->getLabel(), Ctx);
-        }
         BB->swapConditionalSuccessors();
-      } else {
+        auto L = BC.scopeLock();
+        MIB->reverseBranchCondition(*CondBranch, TSuccessor->getLabel(), Ctx);
+        return true;
+      };
+
+      // Check whether the next block is a "taken" target and try to swap it
+      // with a "fall-through" target.
+      if (TSuccessor == NextBB && swapSuccessors())
+        continue;
+
+      // Update conditional branch destination if needed.
+      if (MIB->getTargetSymbol(*CondBranch) != TSuccessor->getLabel()) {
         auto L = BC.scopeLock();
         MIB->replaceBranchTarget(*CondBranch, TSuccessor->getLabel(), Ctx);
       }
-      if (TSuccessor == FSuccessor)
-        BB->removeDuplicateConditionalSuccessor(CondBranch);
-      if (!NextBB ||
-          ((NextBB != TSuccessor || !IsSupported) && NextBB != FSuccessor)) {
-        // If one of the branches is guaranteed to be "long" while the other
-        // could be "short", then prioritize short for "taken". This will
-        // generate a sequence 1 byte shorter on x86.
-        if (IsSupported && BC.isX86() &&
-            TSuccessor->getFragmentNum() != FSuccessor->getFragmentNum() &&
-            BB->getFragmentNum() != TSuccessor->getFragmentNum()) {
-          std::swap(TSuccessor, FSuccessor);
-          {
-            auto L = BC.scopeLock();
-            MIB->reverseBranchCondition(*CondBranch, TSuccessor->getLabel(),
-                                        Ctx);
-          }
-          BB->swapConditionalSuccessors();
-        }
-        BB->addBranchInstruction(FSuccessor);
+
+      // No need for the unconditional branch.
+      if (FSuccessor == NextBB)
+        continue;
+
+      if (BC.isX86()) {
+        // We are going to generate two branches. Check if their targets are in
+        // the same fragment as this block. If only one target is in the same
+        // fragment, make it the destination of the conditional branch. There
+        // is a chance it will be a short branch which takes 5 bytes fewer than
+        // a long conditional branch. For unconditional branch, the difference
+        // is 4 bytes.
+        if (BB->getFragmentNum() != TSuccessor->getFragmentNum() &&
+            BB->getFragmentNum() == FSuccessor->getFragmentNum())
+          swapSuccessors();
       }
+
+      BB->addBranchInstruction(FSuccessor);
     }
     // Cases where the number of successors is 0 (block ends with a
     // terminator) or more than 2 (switch table) don't require branch

bolt/lib/Core/BinaryFunction.cpp

@@ -427,6 +427,8 @@ void ReorderFunctions::runOnFunctions(BinaryContext &BC) {
 
   reorder(std::move(Clusters), BFs);
 
+  BC.HasFinalizedFunctionOrder = true;
+
   std::unique_ptr<std::ofstream> FuncsFile;
   if (!opts::GenerateFunctionOrderFile.empty()) {
     FuncsFile = std::make_unique<std::ofstream>(opts::GenerateFunctionOrderFile,

bolt/lib/Passes/ReorderFunctions.cpp

@@ -92,6 +92,9 @@ static cl::opt<SplitFunctionsStrategy> SplitStrategy(
     cl::values(clEnumValN(SplitFunctionsStrategy::Profile2, "profile2",
                           "split each function into a hot and cold fragment "
                           "using profiling information")),
+    cl::values(clEnumValN(SplitFunctionsStrategy::CDSplit, "cdsplit",
+                          "split each function into a hot, warm, and cold "
+                          "fragment using profiling information")),
     cl::values(clEnumValN(
         SplitFunctionsStrategy::Random2, "random2",
         "split each function into a hot and cold fragment at a randomly chosen "
@@ -126,7 +129,7 @@ struct SplitProfile2 final : public SplitStrategy {
     return BF.hasValidProfile() && hasFullProfile(BF) && !allBlocksCold(BF);
   }
 
-  bool keepEmpty() override { return false; }
+  bool compactFragments() override { return true; }
 
   void fragment(const BlockIt Start, const BlockIt End) override {
     for (BinaryBasicBlock *const BB : llvm::make_range(Start, End)) {
@@ -136,14 +139,59 @@ struct SplitProfile2 final : public SplitStrategy {
   }
 };
 
+struct SplitCacheDirected final : public SplitStrategy {
+  using BasicBlockOrder = BinaryFunction::BasicBlockOrderType;
+
+  bool canSplit(const BinaryFunction &BF) override {
+    return BF.hasValidProfile() && hasFullProfile(BF) && !allBlocksCold(BF);
+  }
+
+  // When some functions are hot-warm split and others are hot-warm-cold split,
+  // we do not want to change the fragment numbers of the blocks in the hot-warm
+  // split functions.
+  bool compactFragments() override { return false; }
+
+  void fragment(const BlockIt Start, const BlockIt End) override {
+    BasicBlockOrder BlockOrder(Start, End);
+    BinaryFunction &BF = *BlockOrder.front()->getFunction();
+
+    size_t BestSplitIndex = findSplitIndex(BF, BlockOrder);
+
+    // Assign fragments based on the computed best split index.
+    // All basic blocks with index up to the best split index become hot.
+    // All remaining blocks are warm / cold depending on if count is
+    // greater than 0 or not.
+    FragmentNum Main(0);
+    FragmentNum Cold(1);
+    FragmentNum Warm(2);
+    for (size_t Index = 0; Index < BlockOrder.size(); Index++) {
+      BinaryBasicBlock *BB = BlockOrder[Index];
+      if (Index <= BestSplitIndex)
+        BB->setFragmentNum(Main);
+      else
+        BB->setFragmentNum(BB->getKnownExecutionCount() > 0 ? Warm : Cold);
+    }
+  }
+
+private:
+  /// Find the best index for splitting. The returned value is the index of the
+  /// last hot basic block. Hence, "no splitting" is equivalent to returning the
+  /// value which is one less than the size of the function.
+  size_t findSplitIndex(const BinaryFunction &BF,
+                        const BasicBlockOrder &BlockOrder) {
+    // Placeholder: hot-warm split after entry block.
+    return 0;
+  }
+};
+
 struct SplitRandom2 final : public SplitStrategy {
   std::minstd_rand0 Gen;
 
   SplitRandom2() : Gen(opts::RandomSeed.getValue()) {}
 
   bool canSplit(const BinaryFunction &BF) override { return true; }
 
-  bool keepEmpty() override { return false; }
+  bool compactFragments() override { return true; }
 
   void fragment(const BlockIt Start, const BlockIt End) override {
     using DiffT = typename std::iterator_traits<BlockIt>::difference_type;
@@ -170,7 +218,7 @@ struct SplitRandomN final : public SplitStrategy {
 
   bool canSplit(const BinaryFunction &BF) override { return true; }
 
-  bool keepEmpty() override { return false; }
+  bool compactFragments() override { return true; }
 
   void fragment(const BlockIt Start, const BlockIt End) override {
     using DiffT = typename std::iterator_traits<BlockIt>::difference_type;
@@ -217,10 +265,10 @@ struct SplitRandomN final : public SplitStrategy {
 struct SplitAll final : public SplitStrategy {
   bool canSplit(const BinaryFunction &BF) override { return true; }
 
-  bool keepEmpty() override {
+  bool compactFragments() override {
     // Keeping empty fragments allows us to test, that empty fragments do not
     // generate symbols.
-    return true;
+    return false;
   }
 
   void fragment(const BlockIt Start, const BlockIt End) override {
@@ -246,10 +294,26 @@ void SplitFunctions::runOnFunctions(BinaryContext &BC) {
   if (!opts::SplitFunctions)
     return;
 
+  // If split strategy is not CDSplit, then a second run of the pass is not
+  // needed after function reordering.
+  if (BC.HasFinalizedFunctionOrder &&
+      opts::SplitStrategy != SplitFunctionsStrategy::CDSplit)
+    return;
+
   std::unique_ptr<SplitStrategy> Strategy;
   bool ForceSequential = false;
 
   switch (opts::SplitStrategy) {
+  case SplitFunctionsStrategy::CDSplit:
+    // CDSplit runs two splitting passes: hot-cold splitting (SplitPrfoile2)
+    // before function reordering and hot-warm-cold splitting
+    // (SplitCacheDirected) after function reordering.
+    if (BC.HasFinalizedFunctionOrder)
+      Strategy = std::make_unique<SplitCacheDirected>();
+    else
+      Strategy = std::make_unique<SplitProfile2>();
+    opts::AggressiveSplitting = true;
+    break;
   case SplitFunctionsStrategy::Profile2:
     Strategy = std::make_unique<SplitProfile2>();
     break;
@@ -382,7 +446,7 @@ void SplitFunctions::splitFunction(BinaryFunction &BF, SplitStrategy &S) {
     CurrentFragment = BB->getFragmentNum();
   }
 
-  if (!S.keepEmpty()) {
+  if (S.compactFragments()) {
     FragmentNum CurrentFragment = FragmentNum::main();
     FragmentNum NewFragment = FragmentNum::main();
     for (BinaryBasicBlock *const BB : NewLayout) {
@@ -394,7 +458,7 @@ void SplitFunctions::splitFunction(BinaryFunction &BF, SplitStrategy &S) {
     }
   }
 
-  BF.getLayout().update(NewLayout);
+  const bool LayoutUpdated = BF.getLayout().update(NewLayout);
 
   // For shared objects, invoke instructions and corresponding landing pads
   // have to be placed in the same fragment. When we split them, create
@@ -404,7 +468,7 @@ void SplitFunctions::splitFunction(BinaryFunction &BF, SplitStrategy &S) {
     Trampolines = createEHTrampolines(BF);
 
   // Check the new size to see if it's worth splitting the function.
-  if (BC.isX86() && BF.isSplit()) {
+  if (BC.isX86() && LayoutUpdated) {
     std::tie(HotSize, ColdSize) = BC.calculateEmittedSize(BF);
     LLVM_DEBUG(dbgs() << "Estimated size for function " << BF
                       << " post-split is <0x" << Twine::utohexstr(HotSize)
@@ -431,6 +495,11 @@ void SplitFunctions::splitFunction(BinaryFunction &BF, SplitStrategy &S) {
       SplitBytesCold += ColdSize;
     }
   }
+
+  // Fix branches if the splitting decision of the pass after function
+  // reordering is different from that of the pass before function reordering.
+  if (LayoutUpdated && BC.HasFinalizedFunctionOrder)
+    BF.fixBranches();
 }
 
 SplitFunctions::TrampolineSetType

bolt/lib/Passes/SplitFunctions.cpp

@@ -430,6 +430,13 @@ void BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
   Manager.registerPass(
       std::make_unique<ReorderFunctions>(PrintReorderedFunctions));
 
+  // This is the second run of the SplitFunctions pass required by certain
+  // splitting strategies (e.g. cdsplit). Running the SplitFunctions pass again
+  // after ReorderFunctions allows the finalized function order to be utilized
+  // to make more sophisticated splitting decisions, like hot-warm-cold
+  // splitting.
+  Manager.registerPass(std::make_unique<SplitFunctions>(PrintSplit));
+
   // Print final dyno stats right while CFG and instruction analysis are intact.
   Manager.registerPass(
       std::make_unique<DynoStatsPrintPass>(

bolt/lib/Rewrite/BinaryPassManager.cpp

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-suspicious-enum-usage %t -- -config="{CheckOptions: {bugprone-suspicious-enum-usage.StrictMode: true}}" --
+// RUN: %check_clang_tidy -std=c++17 %s bugprone-suspicious-enum-usage %t -- -config="{CheckOptions: {bugprone-suspicious-enum-usage.StrictMode: true}}" --
 
 enum A {
   A = 1,
@@ -71,7 +71,7 @@ int trigger() {
   unsigned p = R;
   PP pp = Q;
   p |= pp;
-  
+
   enum X x = Z;
   p = x | Z;
   return 0;

clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-enum-usage-strict.cpp

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-suspicious-enum-usage %t -- -config="{CheckOptions: {bugprone-suspicious-enum-usage.StrictMode: false}}" --
+// RUN: %check_clang_tidy -std=c++17 %s bugprone-suspicious-enum-usage %t -- -config="{CheckOptions: {bugprone-suspicious-enum-usage.StrictMode: false}}"
 
 enum Empty {
 };
@@ -79,7 +79,7 @@ int dont_trigger() {
   int d = c | H, e = b * a;
   a = B | C;
   b = X | Z;
-  
+
   if (Tuesday != Monday + 1 ||
       Friday - Thursday != 1 ||
       Sunday + Wednesday == (Sunday | Wednesday))