[PATCH] D29512: [PGO] Directory name stripping in global identifier for static functions

[Rong Xu via Phabricator via llvm-commits]

xur created this revision.
Herald added a subscriber: mehdi_amini.

Current internal option -static-func-full-module-prefix strips all the directory path the profile counter names for static functions. The default of this option is true. This is problematic:

(1) it creates linker errors for profile-generation compilation, exposed in our
internal benchmarks. We are seeing messages like
"warning: relocation refers to discarded section".
This is due to the name conflicts after the stripping.

(2) the stripping only applies to getPGOFuncName. This can create inconsistency in thin-lto's static promotion, which in turn causes problems in thin-lto's indirect-call-promotion (missing targets).

This patch turns the default value for -static-func-full-module-prefix to false.

The second part of the patch is to have an alternative implementation under the internal option -static-func-strip-dirname-prefix=<value>

This options specifies level of directories to be stripped from the source 
filename. Using a large value as the parameter has the same effect as
-static-func-full-module-prefix.

We plan to retire -static-func-full-module-prefix in the future.


https://reviews.llvm.org/D29512

Files:
  lib/IR/Globals.cpp
  lib/ProfileData/InstrProf.cpp
  test/Transforms/PGOProfile/statics_counter_naming.ll


Index: test/Transforms/PGOProfile/statics_counter_naming.ll
===================================================================
--- test/Transforms/PGOProfile/statics_counter_naming.ll
+++ test/Transforms/PGOProfile/statics_counter_naming.ll
@@ -1,5 +1,7 @@
-; RUN: opt %s -pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
-; RUN: opt %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
+; RUN: opt %s -pgo-instr-gen -static-func-full-module-prefix=false -S | FileCheck %s --check-prefix=GEN
+; RUN: opt %s -passes=pgo-instr-gen -static-func-full-module-prefix=false -S | FileCheck %s --check-prefix=GEN
+; RUN: opt %s --pgo-instr-gen -static-func-strip-dirname-prefix=1000 -S | FileCheck %s --check-prefix=GEN
+; RUN: opt %s -passes=pgo-instr-gen -static-func-strip-dirname-prefix=1000 -S | FileCheck %s --check-prefix=GEN
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
Index: lib/ProfileData/InstrProf.cpp
===================================================================
--- lib/ProfileData/InstrProf.cpp
+++ lib/ProfileData/InstrProf.cpp
@@ -29,7 +29,7 @@
 using namespace llvm;
 
 static cl::opt<bool> StaticFuncFullModulePrefix(
-    "static-func-full-module-prefix", cl::init(false),
+    "static-func-full-module-prefix", cl::init(true),
     cl::desc("Use full module build paths in the profile counter names for "
              "static functions."));
 
Index: lib/IR/Globals.cpp
===================================================================
--- lib/IR/Globals.cpp
+++ lib/IR/Globals.cpp
@@ -24,13 +24,19 @@
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/Path.h"
 #include "LLVMContextImpl.h"
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 //                            GlobalValue Class
 //===----------------------------------------------------------------------===//
 
+static cl::opt<unsigned> StaticFuncStripDirNamePrefix(
+    "static-func-strip-dirname-prefix", cl::init(0),
+    cl::desc("Strip specified level of directory name from source paths in "
+             "the profile counter names for static functions."));
+
 // GlobalValue should be a Constant, plus a type, a module, some flags, and an
 // intrinsic ID. Add an assert to prevent people from accidentally growing
 // GlobalValue while adding flags.
@@ -113,6 +119,24 @@
   }
 }
 
+// Strip NumPrefix level of directory name from PathNameStr. If the number of
+// directory separators is less then NumPrefix, strip all the directories and
+// leave base file name only.
+static StringRef stripDirPrefix(StringRef PathNameStr, uint32_t NumPrefix) {
+  uint32_t Count = NumPrefix; 
+  uint32_t Pos = 0, LastPos = 0;
+  for (auto & CI : PathNameStr) {
+    ++Pos; 
+    if (llvm::sys::path::is_separator(CI)) {
+      LastPos = Pos;
+      --Count;
+    }
+    if (Count == 0)
+      break;
+  }
+  return PathNameStr.substr(LastPos);
+}
+
 std::string GlobalValue::getGlobalIdentifier(StringRef Name,
                                              GlobalValue::LinkageTypes Linkage,
                                              StringRef FileName) {
@@ -129,10 +153,13 @@
     // Do not include the full path in the file name since there's no guarantee
     // that it will stay the same, e.g., if the files are checked out from
     // version control in different locations.
-    if (FileName.empty())
+    if (FileName.empty()) 
       NewName = NewName.insert(0, "<unknown>:");
-    else
+    else {
+      if (StaticFuncStripDirNamePrefix != 0)
+        FileName = stripDirPrefix(FileName, StaticFuncStripDirNamePrefix);
       NewName = NewName.insert(0, FileName.str() + ":");
+    }
   }
   return NewName;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D29512.87009.patch
Type: text/x-patch
Size: 3819 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170203/fd338857/attachment.bin>