resolve review feedback

Author: mingmingl-llvm

llvm/include/llvm/CodeGen/MachineFunction.h

@@ -91,7 +91,7 @@ template <> struct ilist_callback_traits<MachineBasicBlock> {
 // The hotness of static data tracked by a MachineFunction and not represented
 // as a global object in the module IR / MIR. Typical examples are
 // MachineJumpTableInfo and MachineConstantPool.
-enum class DataHotness {
+enum class MachineFunctionDataHotness {
   Unknown,
   Cold,
   Hot,

llvm/include/llvm/CodeGen/MachineJumpTableInfo.h

@@ -28,15 +28,15 @@ namespace llvm {
 class MachineBasicBlock;
 class DataLayout;
 class raw_ostream;
-enum class DataHotness;
+enum class MachineFunctionDataHotness;
 
 /// MachineJumpTableEntry - One jump table in the jump table info.
 ///
 struct MachineJumpTableEntry {
   /// MBBs - The vector of basic blocks from which to create the jump table.
   std::vector<MachineBasicBlock*> MBBs;
 
-  DataHotness Hotness;
+  MachineFunctionDataHotness Hotness;
 
   explicit MachineJumpTableEntry(const std::vector<MachineBasicBlock *> &M);
 };
@@ -109,7 +109,10 @@ class MachineJumpTableInfo {
     return JumpTables;
   }
 
-  void updateJumpTableHotness(size_t JTI, DataHotness Hotness);
+  // Update machine jump table entry's hotness. Return true if the hotness is
+  // updated.
+  bool updateJumpTableEntryHotness(size_t JTI,
+                                   MachineFunctionDataHotness Hotness);
 
   /// RemoveJumpTable - Mark the specific index as being dead.  This will
   /// prevent it from being emitted.

llvm/include/llvm/CodeGen/Passes.h

@@ -71,7 +71,7 @@ namespace llvm {
   /// using profile information.
   MachineFunctionPass *createMachineFunctionSplitterPass();
 
-  /// createStaticDataSplitterPass - This pass partions static data sections
+  /// createStaticDataSplitterPass - This pass partitions a static data section
   /// into a hot and cold section using profile information.
   MachineFunctionPass *createStaticDataSplitterPass();
 

llvm/lib/CodeGen/MachineFunction.cpp

@@ -1293,7 +1293,7 @@ const unsigned MachineFunction::DebugOperandMemNumber = 1000000;
 
 MachineJumpTableEntry::MachineJumpTableEntry(
     const std::vector<MachineBasicBlock *> &MBBs)
-    : MBBs(MBBs), Hotness(DataHotness::Unknown) {}
+    : MBBs(MBBs), Hotness(MachineFunctionDataHotness::Unknown) {}
 
 /// Return the size of each entry in the jump table.
 unsigned MachineJumpTableInfo::getEntrySize(const DataLayout &TD) const {
@@ -1344,13 +1344,17 @@ unsigned MachineJumpTableInfo::createJumpTableIndex(
   return JumpTables.size()-1;
 }
 
-void MachineJumpTableInfo::updateJumpTableHotness(size_t JTI,
-                                                  DataHotness Hotness) {
+bool MachineJumpTableInfo::updateJumpTableEntryHotness(
+    size_t JTI, MachineFunctionDataHotness Hotness) {
   assert(JTI < JumpTables.size() && "Invalid JTI!");
-  // Note: recording the largest hotness is important for mergable data (constant
+  // Note record the largest hotness is important for mergable data (constant
   // pools). Even if jump table instances are not merged, record the largest
   // value seen fwiw.
-  JumpTables[JTI].Hotness = std::max(JumpTables[JTI].Hotness, Hotness);
+  if (Hotness <= JumpTables[JTI].Hotness)
+    return false;
+
+  JumpTables[JTI].Hotness = Hotness;
+  return true;
 }
 
 /// If Old is the target of any jump tables, update the jump tables to branch

llvm/lib/CodeGen/StaticDataSplitter.cpp

@@ -6,13 +6,16 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This pass uses profile information to partition static data sections into
-// hot and cold ones. It begins to split jump tables based on profile, and
-// subsequent patches will handle constant pools and other module internal data.
+// The pass uses branch profile data to assign hotness based section qualifiers
+// for the following types of static data:
+// - Jump tables
+// - Constant pools (TODO)
+// - Other module-internal data (TODO)
 //
 // For the original RFC of this pass please see
-// https://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744.
+// https://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/CodeGen/MBFIWrapper.h"
@@ -35,8 +38,16 @@ using namespace llvm;
 STATISTIC(NumHotJumpTables, "Number of hot jump tables seen");
 STATISTIC(NumColdJumpTables, "Number of cold jump tables seen");
 STATISTIC(NumUnknownJumpTables,
-          "Number of jump tables with unknown hotness. Such jump tables will "
-          "be placed in the hot-suffixed section by default.");
+          "Number of jump tables with unknown hotness. Option "
+          "-static-data-default-hotness specifies the hotness.");
+
+static cl::opt<MachineFunctionDataHotness> StaticDataDefaultHotness(
+    "static-data-default-hotness", cl::Hidden,
+    cl::desc("This option specifies the hotness of static data when profile "
+             "information is unavailable"),
+    cl::init(MachineFunctionDataHotness::Hot),
+    cl::values(clEnumValN(MachineFunctionDataHotness::Hot, "hot", "Hot"),
+               clEnumValN(MachineFunctionDataHotness::Cold, "cold", "Cold")));
 
 class StaticDataSplitter : public MachineFunctionPass {
   const MachineBranchProbabilityInfo *MBPI = nullptr;
@@ -74,20 +85,13 @@ bool StaticDataSplitter::runOnMachineFunction(MachineFunction &MF) {
   MBFI = &getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI();
   PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
 
-  // Split jump tables based on profile information. Subsequent patches will
-  // handle other data types like constant pools, module-internal data, etc.
   return splitJumpTables(MF);
 }
 
 bool StaticDataSplitter::splitJumpTablesWithProfiles(
     MachineFunction &MF, MachineJumpTableInfo &MJTI) {
   int NumChangedJumpTables = 0;
-  // Regard a jump table as hot by default. If the source and all of destination
-  // blocks are cold, regard the jump table as cold. While a destination block
-  // does not read a jump table (unless it's also a source block), a hot
-  // destination heuristically makes its jump table hot to accommodate for
-  // potential profile data skews (from sampled profiles, for example).
-  DataHotness Hotness = DataHotness::Hot;
+
   for (const auto &MBB : MF) {
     // IMPORTANT, `getJumpTableIndex` is a thin wrapper around per-target
     // interface `TargetInstrInfo::getjumpTableIndex`, and only X86 implements
@@ -97,21 +101,14 @@ bool StaticDataSplitter::splitJumpTablesWithProfiles(
     if (JTI == -1)
       continue;
 
-    bool AllBlocksCold = PSI->isColdBlock(&MBB, MBFI);
+    auto Hotness = MachineFunctionDataHotness::Hot;
 
-    for (const MachineBasicBlock *MBB : MJTI.getJumpTables()[JTI].MBBs)
-      if (!PSI->isColdBlock(MBB, MBFI))
-        AllBlocksCold = false;
+    // Hotness is based on source basic block hotness.
+    if (PSI->isColdBlock(&MBB, MBFI))
+      Hotness = MachineFunctionDataHotness::Cold;
 
-    if (AllBlocksCold) {
-      Hotness = DataHotness::Cold;
-      ++NumColdJumpTables;
-    } else {
-      ++NumHotJumpTables;
-    }
-
-    MF.getJumpTableInfo()->updateJumpTableHotness(JTI, Hotness);
-    ++NumChangedJumpTables;
+    if (MF.getJumpTableInfo()->updateJumpTableEntryHotness(JTI, Hotness))
+      ++NumChangedJumpTables;
   }
   return NumChangedJumpTables > 0;
 }
@@ -121,18 +118,41 @@ bool StaticDataSplitter::splitJumpTables(MachineFunction &MF) {
   if (!MJTI || MJTI->getJumpTables().empty())
     return false;
 
+  const bool ProfileAvailable = PSI && PSI->hasProfileSummary() && MBFI &&
+                                MF.getFunction().hasProfileData();
+  auto statOnExit = llvm::make_scope_exit([&] {
+    if (!AreStatisticsEnabled())
+      return;
+
+    if (!ProfileAvailable) {
+      NumUnknownJumpTables += MJTI->getJumpTables().size();
+      return;
+    }
+
+    for (size_t JTI = 0; JTI < MJTI->getJumpTables().size(); JTI++) {
+      auto Hotness = MJTI->getJumpTables()[JTI].Hotness;
+      if (Hotness == MachineFunctionDataHotness::Hot)
+        NumHotJumpTables++;
+      else {
+        assert(Hotness == MachineFunctionDataHotness::Cold &&
+               "A jump table is either hot or cold when profile information is "
+               "available.");
+        NumColdJumpTables++;
+      }
+    }
+  });
+
   // Place jump tables according to block hotness if function has profile data.
-  if (PSI && PSI->hasProfileSummary() && MBFI &&
-      MF.getFunction().hasProfileData())
+  if (ProfileAvailable)
     return splitJumpTablesWithProfiles(MF, *MJTI);
 
-  // Conservatively place all jump tables in the hot-suffixed section if profile
-  // information for the function is not available, or the target doesn't
-  // implement `TargetInstrInfo::getJumpTableIndex` yet.
+  // If function profile is unavailable (e.g., module not instrumented, or new
+  // code paths lacking samples), -static-data-default-hotness specifies the
+  // hotness.
   for (size_t JTI = 0; JTI < MJTI->getJumpTables().size(); JTI++)
-    MF.getJumpTableInfo()->updateJumpTableHotness(JTI, DataHotness::Hot);
+    MF.getJumpTableInfo()->updateJumpTableEntryHotness(
+        JTI, StaticDataDefaultHotness);
 
-  NumUnknownJumpTables += MJTI->getJumpTables().size();
   return true;
 }