Add a pass to calculate machine function's cfg hash to detect whether… #84145

lifengxiang1025 · 2024-03-06T09:42:03Z

… Propeller's profile is out-dated [1/2]

llvmbot · 2024-03-06T09:42:35Z

@llvm/pr-subscribers-backend-x86

Author: None (lifengxiang1025)

Changes

… Propeller's profile is out-dated [1/2]

Patch is 22.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84145.diff

12 Files Affected:

(modified) llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h (+6)
(added) llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h (+57)
(modified) llvm/include/llvm/CodeGen/Passes.h (+5)
(modified) llvm/include/llvm/InitializePasses.h (+1-1)
(modified) llvm/lib/CodeGen/BasicBlockPathCloning.cpp (+11)
(modified) llvm/lib/CodeGen/BasicBlockSections.cpp (+21-9)
(modified) llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp (+37-11)
(modified) llvm/lib/CodeGen/CMakeLists.txt (+1)
(modified) llvm/lib/CodeGen/CodeGen.cpp (+1)
(added) llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp (+79)
(modified) llvm/lib/CodeGen/TargetPassConfig.cpp (+8)
(added) llvm/test/CodeGen/X86/machine-function-cfg-hash.ll (+81)

diff --git a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
index bba675f1d3eb7d..8707925f0afbaa 100644
--- a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
+++ b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
@@ -100,6 +100,8 @@ class BasicBlockSectionsProfileReader {
   SmallVector<SmallVector<unsigned>>
   getClonePathsForFunction(StringRef FuncName) const;
 
+  uint64_t getCFGHashForFunction(StringRef FuncName) const;
+
 private:
   StringRef getAliasName(StringRef FuncName) const {
     auto R = FuncAliasMap.find(FuncName);
@@ -151,6 +153,8 @@ class BasicBlockSectionsProfileReader {
   // Some functions have alias names. We use this map to find the main alias
   // name which appears in ProgramPathAndClusterInfo as a key.
   StringMap<StringRef> FuncAliasMap;
+
+  StringMap<uint64_t> FuncHashMap;
 };
 
 // Creates a BasicBlockSectionsProfileReader pass to parse the basic block
@@ -206,6 +210,8 @@ class BasicBlockSectionsProfileReaderWrapperPass : public ImmutablePass {
   SmallVector<SmallVector<unsigned>>
   getClonePathsForFunction(StringRef FuncName) const;
 
+  uint64_t getCFGHashForFunction(StringRef FuncName) const;
+
   // Initializes the FunctionNameToDIFilename map for the current module and
   // then reads the profile for the matching functions.
   bool doInitialization(Module &M) override;
diff --git a/llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h b/llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h
new file mode 100644
index 00000000000000..85fd43e19762a6
--- /dev/null
+++ b/llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h
@@ -0,0 +1,57 @@
+//===-- MachineFunctionHashBuilder.h ----------------------------------*-
+// C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains the implementation of pass about calculating machine
+/// function hash.
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_MACHINE_FUNCTION_HASH_BUILDER_H
+#define LLVM_MACHINE_FUNCTION_HASH_BUILDER_H
+
+#include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/Passes.h"
+#include "llvm/InitializePasses.h"
+
+namespace llvm {
+
+class MachineFunctionHashBuilder : public MachineFunctionPass {
+public:
+  static char ID;
+
+  MachineFunctionHashBuilder() : MachineFunctionPass(ID) {
+    initializeMachineFunctionHashBuilderPass(*PassRegistry::getPassRegistry());
+  }
+
+  StringRef getPassName() const override {
+    return "Calculate machine function hash.";
+  }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesAll();
+    MachineFunctionPass::getAnalysisUsage(AU);
+  }
+
+  uint64_t getCFGHash(StringRef Name) {
+    if (!CFGHash.count(Name)) {
+      return 0;
+    }
+    return CFGHash[Name];
+  }
+
+private:
+  void setCFGHash(StringRef Name, uint64_t Hash) { CFGHash[Name] = Hash; }
+  StringMap<uint64_t> CFGHash;
+};
+
+} // namespace llvm
+#endif
\ No newline at end of file
diff --git a/llvm/include/llvm/CodeGen/Passes.h b/llvm/include/llvm/CodeGen/Passes.h
index f850767270a4fd..ab25ec59e0a54c 100644
--- a/llvm/include/llvm/CodeGen/Passes.h
+++ b/llvm/include/llvm/CodeGen/Passes.h
@@ -605,6 +605,11 @@ namespace llvm {
 
   /// Lowers KCFI operand bundles for indirect calls.
   FunctionPass *createKCFIPass();
+
+  /// Calculate machine function hash.
+  extern char &MachineFunctionHashBuilderID;
+
+  MachineFunctionPass *createMachineFunctionHashBuilderPass();
 } // End llvm namespace
 
 #endif
diff --git a/llvm/include/llvm/InitializePasses.h b/llvm/include/llvm/InitializePasses.h
index e4bf3868d00069..c2f5146c7c36c3 100644
--- a/llvm/include/llvm/InitializePasses.h
+++ b/llvm/include/llvm/InitializePasses.h
@@ -191,6 +191,7 @@ void initializeMachineCycleInfoPrinterPassPass(PassRegistry &);
 void initializeMachineCycleInfoWrapperPassPass(PassRegistry &);
 void initializeMachineDominanceFrontierPass(PassRegistry&);
 void initializeMachineDominatorTreePass(PassRegistry&);
+void initializeMachineFunctionHashBuilderPass(PassRegistry &);
 void initializeMachineFunctionPrinterPassPass(PassRegistry&);
 void initializeMachineFunctionSplitterPass(PassRegistry &);
 void initializeMachineLateInstrsCleanupPass(PassRegistry&);
@@ -313,7 +314,6 @@ void initializeWasmEHPreparePass(PassRegistry&);
 void initializeWinEHPreparePass(PassRegistry&);
 void initializeWriteBitcodePassPass(PassRegistry&);
 void initializeXRayInstrumentationPass(PassRegistry&);
-
 } // end namespace llvm
 
 #endif // LLVM_INITIALIZEPASSES_H
diff --git a/llvm/lib/CodeGen/BasicBlockPathCloning.cpp b/llvm/lib/CodeGen/BasicBlockPathCloning.cpp
index 901542e8507bdf..0bcc3117bacd7f 100644
--- a/llvm/lib/CodeGen/BasicBlockPathCloning.cpp
+++ b/llvm/lib/CodeGen/BasicBlockPathCloning.cpp
@@ -37,6 +37,7 @@
 #include "llvm/CodeGen/BasicBlockSectionUtils.h"
 #include "llvm/CodeGen/BasicBlockSectionsProfileReader.h"
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionHashBuilder.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
@@ -219,6 +220,7 @@ INITIALIZE_PASS_BEGIN(
     "Applies path clonings for the -basic-block-sections=list option", false,
     false)
 INITIALIZE_PASS_DEPENDENCY(BasicBlockSectionsProfileReaderWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(MachineFunctionHashBuilder)
 INITIALIZE_PASS_END(
     BasicBlockPathCloning, "bb-path-cloning",
     "Applies path clonings for the -basic-block-sections=list option", false,
@@ -229,6 +231,14 @@ bool BasicBlockPathCloning::runOnMachineFunction(MachineFunction &MF) {
          "BB Sections list not enabled!");
   if (hasInstrProfHashMismatch(MF))
     return false;
+  // Check for cfg drift.
+  if (auto *MFHB = getAnalysisIfAvailable<MachineFunctionHashBuilder>()) {
+    auto &BBSPRWP = getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>();
+    if (MFHB->getCFGHash(MF.getName()) !=
+        BBSPRWP.getCFGHashForFunction(MF.getName())) {
+      return false;
+    }
+  }
 
   return ApplyCloning(MF,
                       getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>()
@@ -238,6 +248,7 @@ bool BasicBlockPathCloning::runOnMachineFunction(MachineFunction &MF) {
 void BasicBlockPathCloning::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   AU.addRequired<BasicBlockSectionsProfileReaderWrapperPass>();
+  AU.addUsedIfAvailable<MachineFunctionHashBuilder>();
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
diff --git a/llvm/lib/CodeGen/BasicBlockSections.cpp b/llvm/lib/CodeGen/BasicBlockSections.cpp
index 09e45ea5794b76..5cc3ec22a3b132 100644
--- a/llvm/lib/CodeGen/BasicBlockSections.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSections.cpp
@@ -73,6 +73,7 @@
 #include "llvm/CodeGen/BasicBlockSectionUtils.h"
 #include "llvm/CodeGen/BasicBlockSectionsProfileReader.h"
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionHashBuilder.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
@@ -133,6 +134,7 @@ INITIALIZE_PASS_BEGIN(
     "into clusters of basic blocks.",
     false, false)
 INITIALIZE_PASS_DEPENDENCY(BasicBlockSectionsProfileReaderWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(MachineFunctionHashBuilder)
 INITIALIZE_PASS_END(BasicBlockSections, "bbsections-prepare",
                     "Prepares for basic block sections, by splitting functions "
                     "into clusters of basic blocks.",
@@ -296,15 +298,24 @@ bool BasicBlockSections::handleBBSections(MachineFunction &MF) {
   if (BBSectionsType == BasicBlockSection::None)
     return false;
 
-  // Check for source drift. If the source has changed since the profiles
-  // were obtained, optimizing basic blocks might be sub-optimal.
-  // This only applies to BasicBlockSection::List as it creates
-  // clusters of basic blocks using basic block ids. Source drift can
-  // invalidate these groupings leading to sub-optimal code generation with
-  // regards to performance.
-  if (BBSectionsType == BasicBlockSection::List &&
-      hasInstrProfHashMismatch(MF))
-    return false;
+  if (BBSectionsType == BasicBlockSection::List) {
+    //  Check for source drift. If the source has changed since the profiles
+    //  were obtained, optimizing basic blocks might be sub-optimal.
+    //  This only applies to BasicBlockSection::List as it creates
+    //  clusters of basic blocks using basic block ids. Source drift can
+    //  invalidate these groupings leading to sub-optimal code generation with
+    //  regards to performance.
+    if (hasInstrProfHashMismatch(MF))
+      return false;
+    // Check for cfg drift.
+    if (auto *MFHB = getAnalysisIfAvailable<MachineFunctionHashBuilder>()) {
+      auto &BBSPRWP = getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>();
+      if (MFHB->getCFGHash(MF.getName()) !=
+          BBSPRWP.getCFGHashForFunction(MF.getName())) {
+        return false;
+      }
+    }
+  }
   // Renumber blocks before sorting them. This is useful for accessing the
   // original layout positions and finding the original fallthroughs.
   MF.RenumberBlocks();
@@ -399,6 +410,7 @@ bool BasicBlockSections::runOnMachineFunction(MachineFunction &MF) {
 void BasicBlockSections::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   AU.addRequired<BasicBlockSectionsProfileReaderWrapperPass>();
+  AU.addUsedIfAvailable<MachineFunctionHashBuilder>();
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
diff --git a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
index 6eef5d2c50a2f9..ab5b49367fc7ac 100644
--- a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
@@ -75,10 +75,16 @@ BasicBlockSectionsProfileReader::getClonePathsForFunction(
   return ProgramPathAndClusterInfo.lookup(getAliasName(FuncName)).ClonePaths;
 }
 
+uint64_t BasicBlockSectionsProfileReader::getCFGHashForFunction(
+    StringRef FuncName) const {
+  auto R = FuncHashMap.find(getAliasName(FuncName));
+  return R != FuncHashMap.end() ? R->second : 0;
+}
 // Reads the version 1 basic block sections profile. Profile for each function
 // is encoded as follows:
 //   m <module_name>
 //   f <function_name_1> <function_name_2> ...
+//   h <hash>
 //   c <bb_id_1> <bb_id_2> <bb_id_3>
 //   c <bb_id_4> <bb_id_5>
 //   ...
@@ -86,17 +92,17 @@ BasicBlockSectionsProfileReader::getClonePathsForFunction(
 // distinguishing profile for internal-linkage functions with the same name. If
 // not specified, it will apply to any function with the same name. Function
 // name specifier (starting with 'f') can specify multiple function name
-// aliases. Basic block clusters are specified by 'c' and specify the cluster of
-// basic blocks, and the internal order in which they must be placed in the same
-// section.
-// This profile can also specify cloning paths which instruct the compiler to
-// clone basic blocks along a path. The cloned blocks are then specified in the
-// cluster information.
-// The following profile lists two cloning paths (starting with 'p') for
-// function bar and places the total 9 blocks within two clusters. The first two
-// blocks of a cloning path specify the edge along which the path is cloned. For
-// instance, path 1 (1 -> 3 -> 4) instructs that 3 and 4 must be cloned along
-// the edge 1->3. Within the given clusters, each cloned block is identified by
+// aliases. Function hash are specified by 'h' to detect whether profile is
+// out-dated. Basic block clusters are specified by 'c' and specify the cluster
+// of basic blocks, and the internal order in which they must be placed in the
+// same section. This profile can also specify cloning paths which instruct the
+// compiler to clone basic blocks along a path. The cloned blocks are then
+// specified in the cluster information. The following profile lists two cloning
+// paths (starting with 'p') for function bar and places the total 9 blocks
+// within two clusters. The first two blocks of a cloning path specify the edge
+// along which the path is cloned. For instance, path 1 (1 -> 3 -> 4) instructs
+// that 3 and 4 must be cloned along the edge 1->3. Within the given clusters,
+// each cloned block is identified by
 // "<original block id>.<clone id>". For instance, 3.1 represents the first
 // clone of block 3. Original blocks are specified just with their block ids. A
 // block cloned multiple times appears with distinct clone ids. The CFG for bar
@@ -198,6 +204,21 @@ Error BasicBlockSectionsProfileReader::ReadV1Profile() {
       DIFilename = "";
       continue;
     }
+    case 'h': { // Machine function cfg hash
+      if (Values.size() != 1) {
+        return createProfileParseError(Twine("invalid hash value: '") + S +
+                                       "'");
+      }
+      if (FI == ProgramPathAndClusterInfo.end())
+        continue;
+      unsigned long long Hash = 0;
+      if (getAsUnsignedInteger(Values.front(), 10, Hash))
+        return createProfileParseError(
+            Twine("unable to parse function hash: '" + Values.front()) +
+            "': unsigned integer expected");
+      FuncHashMap[FI->first()] = static_cast<uint64_t>(Hash);
+      continue;
+    }
     case 'c': // Basic block cluster specifier.
       // Skip the profile when we the profile iterator (FI) refers to the
       // past-the-end element.
@@ -444,6 +465,11 @@ BasicBlockSectionsProfileReaderWrapperPass::getBBSPR() {
   return BBSPR;
 }
 
+uint64_t BasicBlockSectionsProfileReaderWrapperPass::getCFGHashForFunction(
+    StringRef FuncName) const {
+  return BBSPR.getCFGHashForFunction(FuncName);
+}
+
 ImmutablePass *llvm::createBasicBlockSectionsProfileReaderWrapperPass(
     const MemoryBuffer *Buf) {
   return new BasicBlockSectionsProfileReaderWrapperPass(Buf);
diff --git a/llvm/lib/CodeGen/CMakeLists.txt b/llvm/lib/CodeGen/CMakeLists.txt
index e02c1d6417e077..250db791597124 100644
--- a/llvm/lib/CodeGen/CMakeLists.txt
+++ b/llvm/lib/CodeGen/CMakeLists.txt
@@ -119,6 +119,7 @@ add_llvm_component_library(LLVMCodeGen
   MachineDominators.cpp
   MachineFrameInfo.cpp
   MachineFunction.cpp
+  MachineFunctionHashBuilder.cpp
   MachineFunctionPass.cpp
   MachineFunctionPrinterPass.cpp
   MachineFunctionSplitter.cpp
diff --git a/llvm/lib/CodeGen/CodeGen.cpp b/llvm/lib/CodeGen/CodeGen.cpp
index 544f1b7f593531..967aaf1276882e 100644
--- a/llvm/lib/CodeGen/CodeGen.cpp
+++ b/llvm/lib/CodeGen/CodeGen.cpp
@@ -142,4 +142,5 @@ void llvm::initializeCodeGen(PassRegistry &Registry) {
   initializeWasmEHPreparePass(Registry);
   initializeWinEHPreparePass(Registry);
   initializeXRayInstrumentationPass(Registry);
+  initializeMachineFunctionHashBuilderPass(Registry);
 }
diff --git a/llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp b/llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp
new file mode 100644
index 00000000000000..d7cf06d439b5bc
--- /dev/null
+++ b/llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp
@@ -0,0 +1,79 @@
+//===-- MachineFunctionHashBuilder.cpp ----------------------------------*-
+// C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains the implementation of pass about calculating machine
+/// function hash.
+//===----------------------------------------------------------------------===//
+#include "llvm/CodeGen/MachineFunctionHashBuilder.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MD5.h"
+#include "llvm/Support/raw_ostream.h"
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+
+using namespace llvm;
+static cl::opt<bool>
+    MFCFGHashDump("mf-cfg-hash-dump",
+                  cl::desc("Dump machine function's control flow grpah hash"),
+                  cl::init(false), cl::Hidden);
+
+// Calculate machine function hash in level order traversal.
+// For each machine basic block, using its mbb's BaseID,
+// size of successors and  successors' mbb's BaseID to update hash.
+// These informations can make graph unique.
+static uint64_t calculateMBBCFGHash(MachineFunction &MF) {
+  std::unordered_set<MachineBasicBlock *> Visited;
+  MD5 Hash;
+  std::queue<MachineBasicBlock *> Q;
+  if (!MF.empty()) {
+    Q.push(&*MF.begin());
+  }
+  while (!Q.empty()) {
+    MachineBasicBlock *Now = Q.front();
+    Q.pop();
+    using namespace llvm::support;
+    uint32_t Value = endian::byte_swap<uint32_t, llvm::endianness::little>(
+        Now->getBBID()->BaseID);
+    uint32_t Size =
+        endian::byte_swap<uint32_t, llvm::endianness::little>(Now->succ_size());
+    Hash.update(llvm::ArrayRef((uint8_t *)&Value, sizeof(Value)));
+    Hash.update(llvm::ArrayRef((uint8_t *)&Size, sizeof(Size)));
+    if (Visited.count(Now)) {
+      continue;
+    }
+    Visited.insert(Now);
+    for (MachineBasicBlock *Succ : Now->successors()) {
+      Q.push(Succ);
+    }
+  }
+  llvm::MD5::MD5Result Result;
+  Hash.final(Result);
+  return Result.low();
+}
+
+bool MachineFunctionHashBuilder::runOnMachineFunction(MachineFunction &MF) {
+  setCFGHash(MF.getName(), calculateMBBCFGHash(MF));
+  if (MFCFGHashDump) {
+    llvm::outs() << "Function name: " << MF.getName().str()
+                 << " Hash: " << getCFGHash(MF.getName()) << "\n";
+  }
+  return true;
+}
+
+char MachineFunctionHashBuilder::ID = 0;
+INITIALIZE_PASS(MachineFunctionHashBuilder, "machine-function-hash",
+                "Calculate machine function hash", false, false)
+char &llvm::MachineFunctionHashBuilderID = MachineFunctionHashBuilder::ID;
+MachineFunctionPass *llvm::createMachineFunctionHashBuilderPass() {
+  return new MachineFunctionHashBuilder();
+}
diff --git a/llvm/lib/CodeGen/TargetPassConfig.cpp b/llvm/lib/CodeGen/TargetPassConfig.cpp
index cf068ece8d4cab..6d65fed80be2ad 100644
--- a/llvm/lib/CodeGen/TargetPassConfig.cpp
+++ b/llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -254,6 +254,11 @@ static cl::opt<bool>
     GCEmptyBlocks("gc-empty-basic-blocks", cl::init(false), cl::Hidden,
                   cl::desc("Enable garbage-collecting empty basic blocks"));
 
+/// Enable calculating machine function's cfg hash.
+static cl::opt<bool> UseMachineFunctionCFGHash(
+    "mf-cfg-hash", cl::init(false), cl::Hidden,
+    cl::desc("Enable calculating machine function's cfg hash."));
+
 /// Allow standard passes to be disabled by command line options. This supports
 /// simple binary flags that either suppress the pass or do nothing.
 /// i.e. -disable-mypass=false has no effect.
@@ -1247,6 +1252,9 @@ void TargetPassConfig::addMachinePasses() {
   // We run the BasicBlockSections pass if either we need BB sections or BB
   // address map (or both).
   if (NeedsBBSections || TM->Options.BBAddrMap) {
+    if (UseMachineFunctionCFGHash) {
+      addPass(llvm::createMachineFunctionHashBuilderPass());
+    }
     if (TM->getBBSectionsType() == llvm::BasicBlockSection::List) {
       addPass(llvm::createBasicBlockSectionsProfileReaderWrapperPass(
           TM->getBBSectionsFuncListBuf()));
diff --git a/llvm/test/CodeGen/X86/machine-function-cfg-hash.ll b/llvm/test/CodeGen/X86/machine-function-cfg-hash.ll
new file mode 100644
index 00000000000000..180a15cab432b8
--- /dev/null
+++ b/llvm/test/CodeGen/X86/machine-function-cfg-hash.ll
@@ -0,0 +1,81 @@
+; RUN: llc < %s -O0 -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=labels \
+; RUN: -mf-cfg-hash=true -mf-cfg-hash-dump=true | FileCheck %s -check-prefix=HASH
+
+;; Foo's cfg is:
+;; 0->2->3
+;;  ->1->3 
+;; Hash is calculated by order: 0 2 2 1 1 1 3 0 3 0.
+; HASH: Function name: _Z3fooi Has...
[truncated]

github-actions · 2024-03-06T09:44:50Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff daf3079222b09683a24dd5b11815f6467ae41b8d 250a38acbaa0b496439847049c23f1f27d108376 -- llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h llvm/include/llvm/CodeGen/Passes.h llvm/include/llvm/InitializePasses.h llvm/lib/CodeGen/BasicBlockPathCloning.cpp llvm/lib/CodeGen/BasicBlockSections.cpp llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp llvm/lib/CodeGen/CodeGen.cpp llvm/lib/CodeGen/TargetPassConfig.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/InitializePasses.h b/llvm/include/llvm/InitializePasses.h
index c2f5146c7c..f19fe265f4 100644
--- a/llvm/include/llvm/InitializePasses.h
+++ b/llvm/include/llvm/InitializePasses.h
@@ -313,7 +313,7 @@ void initializeVirtRegRewriterPass(PassRegistry&);
 void initializeWasmEHPreparePass(PassRegistry&);
 void initializeWinEHPreparePass(PassRegistry&);
 void initializeWriteBitcodePassPass(PassRegistry&);
-void initializeXRayInstrumentationPass(PassRegistry&);
+void initializeXRayInstrumentationPass(PassRegistry &);
 } // end namespace llvm
 
 #endif // LLVM_INITIALIZEPASSES_H

lifengxiang1025 · 2024-03-06T09:51:45Z

Hi @rlavaee
Please review this patch. I plan to add changes to AsmPrinter/objdump/readobj in part2 if this patch is adopted. Thanks.

lifengxiang1025 · 2024-03-06T10:04:48Z

C/C++ code formatter, clang-format found issues in your code.
-void initializeXRayInstrumentationPass(PassRegistry&);
+void initializeXRayInstrumentationPass(PassRegistry &);

It confuses me. I don't change this code.

rlavaee

Sorry for the delay. A first round of review.

rlavaee · 2024-03-13T22:07:00Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+// Calculate machine function hash in level order traversal.
+// For each machine basic block, using its mbb's BaseID,
+// size of successors and  successors' mbb's BaseID to update hash.
+// These informations can make graph unique.


This information

Although I think this sentence is a bit ambiguous. Maybe rephrase to say "The control flow graph is uniquely represented by its level-order traversal". BTW, do you enforce any order on the traversal of the successors? If you don't, it won't be unique.

rlavaee · 2024-03-13T22:11:27Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+      continue;
+    }
+    Visited.insert(Now);
+    for (MachineBasicBlock *Succ : Now->successors()) {


You may want to enforce an ordering here. Maybe based on BaseID.

Thanks. Done.

rlavaee · 2024-03-13T22:22:35Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+// For each machine basic block, using its mbb's BaseID,
+// size of successors and  successors' mbb's BaseID to update hash.
+// These informations can make graph unique.
+static uint64_t calculateMBBCFGHash(MachineFunction &MF) {


This can be const MachineFunction &. Right?

rlavaee · 2024-03-13T22:22:44Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+static uint64_t calculateMBBCFGHash(MachineFunction &MF) {
+  std::unordered_set<MachineBasicBlock *> Visited;
+  MD5 Hash;
+  std::queue<MachineBasicBlock *> Q;


Maybe change to:
if (MF.empty()) return 0;
std::queue<const MachineBasicBlock *> WorkList(&MF.front());
...

rlavaee · 2024-03-13T22:24:03Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+  while (!Q.empty()) {
+    MachineBasicBlock *Now = Q.front();
+    Q.pop();
+    using namespace llvm::support;


Just use support::endian::byte_swap.

rlavaee · 2024-03-13T22:29:00Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+
+bool MachineFunctionHashBuilder::runOnMachineFunction(MachineFunction &MF) {
+  setCFGHash(MF.getName(), calculateMBBCFGHash(MF));
+  if (MFCFGHashDump) {


How important is this option? Maybe we can remove it.

Done. Remove it.

rlavaee · 2024-03-13T22:30:06Z

llvm/lib/CodeGen/TargetPassConfig.cpp

+/// Enable calculating machine function's cfg hash.
+static cl::opt<bool> UseMachineFunctionCFGHash(
+    "mf-cfg-hash", cl::init(false), cl::Hidden,
+    cl::desc("Enable calculating machine function's cfg hash."));


Could you please rename and rephrase the description of this option? It doesn't mention what it will be used for.

rlavaee · 2024-03-13T22:31:30Z

llvm/test/CodeGen/X86/machine-function-cfg-hash.ll

@@ -0,0 +1,81 @@
+; RUN: llc < %s -O0 -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=labels \


Maybe we don't need to add and test the dump option.

rlavaee · 2024-03-13T22:33:24Z

llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h

+  }
+
+  uint64_t getCFGHash(StringRef Name) {
+    if (!CFGHash.count(Name)) {


return CFGHash.lookup(Name);

rlavaee · 2024-03-13T22:33:38Z

llvm/include/llvm/CodeGen/MachineFunctionHashBuilder.h

+    MachineFunctionPass::getAnalysisUsage(AU);
+  }
+
+  uint64_t getCFGHash(StringRef Name) {


Add a function comment.

lifengxiang1025 · 2024-03-18T06:47:41Z

Sorry for the delay. A first round of review.

Thanks! I have updated code based on your code review.

rlavaee · 2024-04-15T18:05:47Z

llvm/lib/CodeGen/MachineFunctionHashBuilder.cpp

+        CurrentBB->getBBID()->BaseID);
+    uint32_t Size = support::endian::byte_swap<uint32_t, endianness::little>(
+        CurrentBB->succ_size());
+    Hash.update(ArrayRef((uint8_t *)&Value, sizeof(Value)));


You may update the hash several times with the same element. A single item may be inserted and visited several times since Visited.count(CurrentBB) is checked later. I think this is unnecessary. You can do this check before pushing items in the worklist. Also, please initialize Visited with the entry block.

For example, the cfg is:

0->2->3 ->1->3

I want to restore the cfg by BB's id and BB's successor's size. So the sequence will be
0(id) 2(successor size) 1(id) 1(successor size) 2(id) 1(successor size) 3(id) 0(successor size) 3(id) 0(successor size).

I don't fully understand the various cases you are trying to tolerate with this hashing. You are only hashing the size and the id of the block. What are the cases you are trying to capture here? Why not hash the contents of the block?

In this mechanism, the cfg hashing will not tolerate a change to the compiler where the blocks had the same CFG but somehow got re-numbered, right?

In this mechanism, the cfg hashing will not tolerate a change to the compiler where the blocks had the same CFG but somehow got re-numbered, right?

Thanks for reply. Yes, you are right.
I think we can divide cfg hashing to three version: loose hash, strict hash, full hash which Bolt do. For example, how about the strict hash is calculated by the size and the id of the block and the full hash is calculated by the contents of the function? More strict hash will cause more compile time.

Can you measure the additional overhead of doing a full/strict hash over a loose hash?

Add a pass to calculate machine function's cfg hash to detect whether…

7f9eca8

… Propeller's profile is out-dated [1/2]

llvmbot added the backend:X86 label Mar 6, 2024

lifengxiang1025 requested a review from rlavaee March 6, 2024 09:42

rlavaee reviewed Mar 13, 2024

View reviewed changes

update based on code review

250a38a

lifengxiang1025 requested a review from rlavaee March 18, 2024 06:47

lifengxiang1025 mentioned this pull request Apr 10, 2024

[Propeller] CFG drift detect #80431

Draft

rlavaee reviewed Apr 15, 2024

View reviewed changes

lifengxiang1025 requested review from rlavaee and tmsri April 21, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a pass to calculate machine function's cfg hash to detect whether… #84145

Add a pass to calculate machine function's cfg hash to detect whether… #84145

lifengxiang1025 commented Mar 6, 2024

llvmbot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024 •

edited

lifengxiang1025 commented Mar 6, 2024 •

edited

lifengxiang1025 commented Mar 6, 2024 •

edited

rlavaee left a comment

rlavaee Mar 13, 2024

rlavaee Mar 13, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

rlavaee Mar 13, 2024

lifengxiang1025 Mar 18, 2024

lifengxiang1025 commented Mar 18, 2024

rlavaee Apr 15, 2024

lifengxiang1025 Apr 18, 2024

tmsri Apr 23, 2024

lifengxiang1025 Apr 24, 2024

tmsri Apr 24, 2024

		@@ -0,0 +1,81 @@
		; RUN: llc < %s -O0 -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=labels \

Add a pass to calculate machine function's cfg hash to detect whether… #84145

Are you sure you want to change the base?

Add a pass to calculate machine function's cfg hash to detect whether… #84145

Conversation

lifengxiang1025 commented Mar 6, 2024

llvmbot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024 • edited

lifengxiang1025 commented Mar 6, 2024 • edited

lifengxiang1025 commented Mar 6, 2024 • edited

rlavaee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifengxiang1025 commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2024 •

edited

lifengxiang1025 commented Mar 6, 2024 •

edited

lifengxiang1025 commented Mar 6, 2024 •

edited