Change related to 2nd comments from nocchijiang

kyulee-com · kyulee-com · commit 7b14e955bd3c · 2024-11-11T01:02:52.000-08:00
- Move isProfitable to offline (llvm-cgdata)
- skip-trim flag to skip trimming codegen data.
  - The default behavior is to trim unnecessary content from the codegen
    data.
  - With this flag enabled, we skip trimming, which is often useful to
    create a synthetic test.
diff --git a/llvm/include/llvm/CGData/StableFunctionMap.h b/llvm/include/llvm/CGData/StableFunctionMap.h
@@ -110,7 +110,7 @@ struct StableFunctionMap {
   size_t size(SizeType Type = UniqueHashCount) const;
 
   /// Finalize the stable function map by trimming content.
-  void finalize();
+  void finalize(bool SkipTrim = false);
 
 private:
   /// Insert a `StableFunctionEntry` into the function map directly. This
diff --git a/llvm/include/llvm/CGData/StableFunctionMapRecord.h b/llvm/include/llvm/CGData/StableFunctionMapRecord.h
@@ -49,7 +49,7 @@ struct StableFunctionMapRecord {
   void deserializeYAML(yaml::Input &YIS);
 
   /// Finalize the stable function map by trimming content.
-  void finalize() { FunctionMap->finalize(); }
+  void finalize(bool SkipTrim = false) { FunctionMap->finalize(SkipTrim); }
 
   /// Merge the stable function map into this one.
   void merge(const StableFunctionMapRecord &Other) {
diff --git a/llvm/lib/CGData/StableFunctionMap.cpp b/llvm/lib/CGData/StableFunctionMap.cpp
@@ -14,11 +14,43 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CGData/StableFunctionMap.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "stable-function-map"
 
 using namespace llvm;
 
+static cl::opt<unsigned>
+    GlobalMergingMinMerges("global-merging-min-merges",
+                           cl::desc("Minimum number of similar functions with "
+                                    "the same hash required for merging."),
+                           cl::init(2), cl::Hidden);
+static cl::opt<unsigned> GlobalMergingMinInstrs(
+    "global-merging-min-instrs",
+    cl::desc("The minimum instruction count required when merging functions."),
+    cl::init(1), cl::Hidden);
+static cl::opt<unsigned> GlobalMergingMaxParams(
+    "global-merging-max-params",
+    cl::desc(
+        "The maximum number of parameters allowed when merging functions."),
+    cl::init(std::numeric_limits<unsigned>::max()), cl::Hidden);
+static cl::opt<unsigned> GlobalMergingParamOverhead(
+    "global-merging-param-overhead",
+    cl::desc("The overhead cost associated with each parameter when merging "
+             "functions."),
+    cl::init(2), cl::Hidden);
+static cl::opt<unsigned>
+    GlobalMergingCallOverhead("global-merging-call-overhead",
+                              cl::desc("The overhead cost associated with each "
+                                       "function call when merging functions."),
+                              cl::init(1), cl::Hidden);
+static cl::opt<unsigned> GlobalMergingExtraThreshold(
+    "global-merging-extra-threshold",
+    cl::desc("An additional cost threshold that must be exceeded for merging "
+             "to be considered beneficial."),
+    cl::init(0), cl::Hidden);
+
 unsigned StableFunctionMap::getIdOrCreateForName(StringRef Name) {
   auto It = NameToId.find(Name);
   if (It != NameToId.end())
@@ -117,7 +149,38 @@ static void removeIdenticalIndexPair(
       SF->IndexOperandHashMap->erase(Pair);
 }
 
-void StableFunctionMap::finalize() {
+static bool isProfitable(
+    const SmallVector<std::unique_ptr<StableFunctionMap::StableFunctionEntry>>
+        &SFS) {
+  unsigned StableFunctionCount = SFS.size();
+  if (StableFunctionCount < GlobalMergingMinMerges)
+    return false;
+
+  unsigned InstCount = SFS[0]->InstCount;
+  if (InstCount < GlobalMergingMinInstrs)
+    return false;
+
+  unsigned ParamCount = SFS[0]->IndexOperandHashMap->size();
+  if (ParamCount > GlobalMergingMaxParams)
+    return false;
+
+  unsigned Benefit = InstCount * (StableFunctionCount - 1);
+  unsigned Cost =
+      (GlobalMergingParamOverhead * ParamCount + GlobalMergingCallOverhead) *
+          StableFunctionCount +
+      GlobalMergingExtraThreshold;
+
+  bool Result = Benefit > Cost;
+  LLVM_DEBUG(dbgs() << "isProfitable: Hash = " << SFS[0]->Hash << ", "
+                    << "StableFunctionCount = " << StableFunctionCount
+                    << ", InstCount = " << InstCount
+                    << ", ParamCount = " << ParamCount
+                    << ", Benefit = " << Benefit << ", Cost = " << Cost
+                    << ", Result = " << (Result ? "true" : "false") << "\n");
+  return Result;
+}
+
+void StableFunctionMap::finalize(bool SkipTrim) {
   for (auto It = HashToFuncs.begin(); It != HashToFuncs.end(); ++It) {
     auto &[StableHash, SFS] = *It;
 
@@ -158,9 +221,15 @@ void StableFunctionMap::finalize() {
       continue;
     }
 
+    if (SkipTrim)
+      continue;
+
     // Trim the index pair that has the same operand hash across
     // stable functions.
     removeIdenticalIndexPair(SFS);
+
+    if (!isProfitable(SFS))
+      HashToFuncs.erase(It);
   }
 
   Finalized = true;
diff --git a/llvm/lib/CodeGen/GlobalMergeFunctions.cpp b/llvm/lib/CodeGen/GlobalMergeFunctions.cpp
@@ -31,30 +31,6 @@ static cl::opt<bool>
                                   "the codegen data generation or use. Local "
                                   "merging is still enabled within a module."),
                          cl::init(false));
-static cl::opt<unsigned> GlobalMergingMinInstrs(
-    "global-merging-min-instrs",
-    cl::desc("The minimum instruction count required when merging functions."),
-    cl::init(1), cl::Hidden);
-static cl::opt<unsigned> GlobalMergingMaxParams(
-    "global-merging-max-params",
-    cl::desc(
-        "The maximum number of parameters allowed when merging functions."),
-    cl::init(std::numeric_limits<unsigned>::max()), cl::Hidden);
-static cl::opt<unsigned> GlobalMergingParamOverhead(
-    "global-merging-param-overhead",
-    cl::desc("The overhead cost associated with each parameter when merging "
-             "functions."),
-    cl::init(2), cl::Hidden);
-static cl::opt<unsigned>
-    GlobalMergingCallOverhead("global-merging-call-overhead",
-                              cl::desc("The overhead cost associated with each "
-                                       "function call when merging functions."),
-                              cl::init(1), cl::Hidden);
-static cl::opt<unsigned> GlobalMergingExtraThreshold(
-    "global-merging-extra-threshold",
-    cl::desc("An additional cost threshold that must be exceeded for merging "
-             "to be considered beneficial."),
-    cl::init(0), cl::Hidden);
 
 STATISTIC(NumMismatchedFunctionHash,
           "Number of mismatched function hash for global merge function");
@@ -442,40 +418,6 @@ static ParamLocsVecTy computeParamInfo(
   return ParamLocsVec;
 }
 
-static bool isProfitable(
-    const SmallVector<std::unique_ptr<StableFunctionMap::StableFunctionEntry>>
-        &SFS,
-    const Function *F) {
-  // No interest if the number of candidates are less than 2.
-  unsigned StableFunctionCount = SFS.size();
-  if (StableFunctionCount < 2)
-    return false;
-
-  unsigned InstCount = SFS[0]->InstCount;
-  if (InstCount < GlobalMergingMinInstrs)
-    return false;
-
-  unsigned ParamCount = SFS[0]->IndexOperandHashMap->size();
-  unsigned TotalParamCount = ParamCount + F->getFunctionType()->getNumParams();
-  if (TotalParamCount > GlobalMergingMaxParams)
-    return false;
-
-  unsigned Benefit = InstCount * (StableFunctionCount - 1);
-  unsigned Cost =
-      (GlobalMergingParamOverhead * ParamCount + GlobalMergingCallOverhead) *
-          StableFunctionCount +
-      GlobalMergingExtraThreshold;
-
-  bool Result = Benefit > Cost;
-  LLVM_DEBUG(dbgs() << "isProfitable: Function = " << F->getName() << ", "
-                    << "StableFunctionCount = " << StableFunctionCount
-                    << ", InstCount = " << InstCount
-                    << ", ParamCount = " << ParamCount
-                    << ", Benefit = " << Benefit << ", Cost = " << Cost
-                    << ", Result = " << (Result ? "true" : "false") << "\n");
-  return Result;
-}
-
 bool GlobalMergeFunc::merge(Module &M, const StableFunctionMap *FunctionMap) {
   bool Changed = false;
 
@@ -488,12 +430,9 @@ bool GlobalMergeFunc::merge(Module &M, const StableFunctionMap *FunctionMap) {
 
   auto ModId = M.getModuleIdentifier();
   for (auto &[Hash, SFS] : FunctionMap->getFunctionMap()) {
-    // Compute the parameter locations based on the unique hash sequences
+    // Parameter locations based on the unique hash sequences
     // across the candidates.
-    auto ParamLocsVec = computeParamInfo(SFS);
-    LLVM_DEBUG(dbgs() << "[GlobalMergeFunc] Merging hash: " << Hash
-                      << " with Params " << ParamLocsVec.size() << "\n");
-
+    std::optional<ParamLocsVecTy> ParamLocsVec;
     Function *MergedFunc = nullptr;
     std::string MergedModId;
     SmallVector<FuncMergeInfo> FuncMergeInfos;
@@ -542,15 +481,17 @@ bool GlobalMergeFunc::merge(Module &M, const StableFunctionMap *FunctionMap) {
         ++NumMismatchedConstHash;
         continue;
       }
+      if (!ParamLocsVec.has_value()) {
+        ParamLocsVec = computeParamInfo(SFS);
+        LLVM_DEBUG(dbgs() << "[GlobalMergeFunc] Merging hash: " << Hash
+                          << " with Params " << ParamLocsVec->size() << "\n");
+      }
       if (!checkConstLocationCompatible(*SF, *FI.IndexInstruction,
-                                        ParamLocsVec)) {
+                                        *ParamLocsVec)) {
         ++NumMismatchedConstHash;
         continue;
       }
 
-      if (!isProfitable(SFS, F))
-        break;
-
       if (MergedFunc) {
         // Check if the matched functions fall into the same (first) module.
         // This module check is not strictly necessary as the functions can move
@@ -583,7 +524,7 @@ bool GlobalMergeFunc::merge(Module &M, const StableFunctionMap *FunctionMap) {
       // the parameters. Populate parameters pointing to the original constants.
       SmallVector<Constant *> Params;
       SmallVector<Type *> ParamTypes;
-      for (auto &ParamLocs : ParamLocsVec) {
+      for (auto &ParamLocs : *ParamLocsVec) {
         assert(!ParamLocs.empty());
         auto &[InstIndex, OpndIndex] = ParamLocs[0];
         auto *Inst = FMI.IndexInstruction->lookup(InstIndex);
@@ -594,7 +535,7 @@ bool GlobalMergeFunc::merge(Module &M, const StableFunctionMap *FunctionMap) {
 
       // Create a merged function derived from the current function.
       Function *MergedFunc =
-          createMergedFunction(FMI, ParamTypes, ParamLocsVec);
+          createMergedFunction(FMI, ParamTypes, *ParamLocsVec);
 
       LLVM_DEBUG({
         dbgs() << "[GlobalMergeFunc] Merged function (hash:" << FMI.SF->Hash
diff --git a/llvm/lib/CodeGen/TargetPassConfig.cpp b/llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -141,7 +141,7 @@ static cl::opt<RunOutliner> EnableMachineOutliner(
                           "Disable all outlining"),
                // Sentinel value for unspecified option.
                clEnumValN(RunOutliner::AlwaysOutline, "", "")));
-cl::opt<bool> EnableGlobalMergeFunc(
+static cl::opt<bool> EnableGlobalMergeFunc(
     "enable-global-merge-func", cl::Hidden,
     cl::desc("Enable global merge functions that are based on hash function"));
 // Disable the pass to fix unwind information. Whether the pass is included in
diff --git a/llvm/test/tools/llvm-cgdata/merge-combined-funcmap-hashtree.test b/llvm/test/tools/llvm-cgdata/merge-combined-funcmap-hashtree.test
@@ -21,7 +21,7 @@ RUN: sed "s/<RAW_2_BYTES>/$(cat %t/raw-funcmap-bytes.txt)/g" %t/merge-both-hasht
 RUN: llc -filetype=obj -mtriple arm64-apple-darwin %t/merge-both-hashtree-funcmap.ll -o %t/merge-both-hashtree-funcmap.o
 
 # Merge an object file having cgdata (__llvm_outline and __llvm_merge)
-RUN: llvm-cgdata -m %t/merge-both-hashtree-funcmap.o -o %t/merge-both-hashtree-funcmap.cgdata
+RUN: llvm-cgdata -m --skip-trim %t/merge-both-hashtree-funcmap.o -o %t/merge-both-hashtree-funcmap.cgdata
 RUN: llvm-cgdata -s %t/merge-both-hashtree-funcmap.cgdata | FileCheck %s
 
 CHECK: Outlined hash tree:
diff --git a/llvm/test/tools/llvm-cgdata/merge-funcmap-archive.test b/llvm/test/tools/llvm-cgdata/merge-funcmap-archive.test
@@ -21,7 +21,7 @@ RUN: llc -filetype=obj -mtriple arm64-apple-darwin %t/merge-2.ll -o %t/merge-2.o
 RUN: llvm-ar rcs %t/merge-archive.a %t/merge-1.o %t/merge-2.o
 
 # Merge the archive into the codegen data file.
-RUN: llvm-cgdata --merge %t/merge-archive.a -o %t/merge-archive.cgdata
+RUN: llvm-cgdata --merge --skip-trim %t/merge-archive.a -o %t/merge-archive.cgdata
 RUN: llvm-cgdata --show %t/merge-archive.cgdata | FileCheck %s
 
 RUN: llvm-cgdata --show %t/merge-archive.cgdata| FileCheck %s
diff --git a/llvm/test/tools/llvm-cgdata/merge-funcmap-concat.test b/llvm/test/tools/llvm-cgdata/merge-funcmap-concat.test
@@ -15,7 +15,7 @@ RUN: od -t x1 -j 32 -An %t/raw-2.cgdata | tr -d '\n\r\t' | sed 's/[ ]*$//' | sed
 RUN: sed "s/<RAW_2_BYTES>/$(cat %t/raw-2-bytes.txt)/g" %t/merge-concat-template-2.ll > %t/merge-concat.ll
 
 RUN: llc -filetype=obj -mtriple arm64-apple-darwin %t/merge-concat.ll -o %t/merge-concat.o
-RUN: llvm-cgdata --merge %t/merge-concat.o -o %t/merge-concat.cgdata
+RUN: llvm-cgdata --merge --skip-trim %t/merge-concat.o -o %t/merge-concat.cgdata
 RUN: llvm-cgdata --show %t/merge-concat.cgdata | FileCheck %s
 
 CHECK: Stable function map:
diff --git a/llvm/test/tools/llvm-cgdata/merge-funcmap-double.test b/llvm/test/tools/llvm-cgdata/merge-funcmap-double.test
@@ -18,7 +18,7 @@ RUN: sed "s/<RAW_2_BYTES>/$(cat %t/raw-2-bytes.txt)/g" %t/merge-2-template.ll >
 RUN: llc -filetype=obj -mtriple arm64-apple-darwin %t/merge-2.ll -o %t/merge-2.o
 
 # Merge two object files into the codegen data file.
-RUN: llvm-cgdata --merge %t/merge-1.o %t/merge-2.o -o %t/merge.cgdata
+RUN: llvm-cgdata --merge --skip-trim %t/merge-1.o %t/merge-2.o -o %t/merge.cgdata
 
 RUN: llvm-cgdata --show %t/merge.cgdata | FileCheck %s
 CHECK: Stable function map:
diff --git a/llvm/test/tools/llvm-cgdata/merge-funcmap-single.test b/llvm/test/tools/llvm-cgdata/merge-funcmap-single.test
@@ -13,7 +13,7 @@ RUN: sed "s/<RAW_1_BYTES>/$(cat %t/raw-single-bytes.txt)/g" %t/merge-single-temp
 RUN: llc -filetype=obj -mtriple arm64-apple-darwin %t/merge-single.ll -o %t/merge-single.o
 
 # Merge an object file having cgdata (__llvm_merge)
-RUN: llvm-cgdata -m %t/merge-single.o -o %t/merge-single.cgdata
+RUN: llvm-cgdata -m --skip-trim %t/merge-single.o -o %t/merge-single.cgdata
 RUN: llvm-cgdata -s %t/merge-single.cgdata | FileCheck %s
 CHECK: Stable function map:
 CHECK-NEXT:  Unique hash Count: 1
diff --git a/llvm/tools/llvm-cgdata/Opts.td b/llvm/tools/llvm-cgdata/Opts.td
@@ -24,6 +24,7 @@ def : F<"m", "Alias for --merge">, Alias<merge>, Group<action_group>;
 
 // Additional options
 def cgdata_version : FF<"cgdata-version", "Display the cgdata version">;
+def skip_trim : FF<"skip-trim", "Skip trimming content when merging the cgdata">;
 def output : Option<["--"], "output", KIND_SEPARATE>,
              HelpText<"Specify the name for the output file to be created">, MetaVarName<"<file>">;
 def : JoinedOrSeparate<["-"], "o">, Alias<output>, MetaVarName<"<file>">, HelpText<"Alias for --output">;
diff --git a/llvm/tools/llvm-cgdata/llvm-cgdata.cpp b/llvm/tools/llvm-cgdata/llvm-cgdata.cpp
@@ -76,6 +76,7 @@ static StringRef ToolName;
 static StringRef OutputFilename = "-";
 static StringRef Filename;
 static bool ShowCGDataVersion;
+static bool SkipTrim;
 static CGDataAction Action;
 static std::optional<CGDataFormat> OutputFormat;
 static std::vector<std::string> InputFilenames;
@@ -214,7 +215,7 @@ static int merge_main(int argc, const char *argv[]) {
   if (!Result)
     exitWithError("failed to merge codegen data files.");
 
-  GlobalFunctionMapRecord.finalize();
+  GlobalFunctionMapRecord.finalize(SkipTrim);
 
   CodeGenDataWriter Writer;
   if (!GlobalOutlineRecord.empty())
@@ -301,6 +302,7 @@ static void parseArgs(int argc, char **argv) {
   }
 
   ShowCGDataVersion = Args.hasArg(OPT_cgdata_version);
+  SkipTrim = Args.hasArg(OPT_skip_trim);
 
   if (opt::Arg *A = Args.getLastArg(OPT_format)) {
     StringRef OF = A->getValue();
diff --git a/llvm/unittests/CGData/StableFunctionMapTest.cpp b/llvm/unittests/CGData/StableFunctionMapTest.cpp
@@ -108,8 +108,8 @@ TEST(StableFunctionMap, Finalize2) {
 
 TEST(StableFunctionMap, Finalize3) {
   StableFunctionMap Map;
-  StableFunction Func1{1, "Func1", "Mod1", 2, {{{0, 1}, 3}, {{1, 1}, 1}}};
-  StableFunction Func2{1, "Func2", "Mod2", 2, {{{0, 1}, 2}, {{1, 1}, 1}}};
+  StableFunction Func1{1, "Func1", "Mod1", 12, {{{0, 1}, 3}, {{1, 1}, 1}}};
+  StableFunction Func2{1, "Func2", "Mod2", 12, {{{0, 1}, 2}, {{1, 1}, 1}}};
   Map.insert(Func1);
   Map.insert(Func2);
 
