[PGO] Don't include full file path in static function profile counter names

[jakev]

Summary

Currently, the full build path gets included in the name of static function profile counters. When the training build and the profile guided build are in different directories, we end up just dropping the profile data and get a warning about unprofiled functions. Using only the filename in the counter prevents this problem, but seems good only as a short term solution.

[davidxl]

I think this should be controlled by an internal option -- I think default can be stripping the path as done in this patch: it is unlikely that two static functions have source files with the same name and the same cfg hash.

[silvas]

For posterity, I want to expand a bit on Jake's explanation:

When the training build and the profile guided build are in different directories, we end up just dropping the profile data and get a warning about unprofiled functions.

To expand on Jake's explanation of this issue, the issue we ran into when building one of our games is actually slightly more complicated than he described. The main complication is that the build generates source files into the build directory. Thus, different build directories (as one might use for separate prof-gen and prof-use builds) cause issues.

Concretely:
During the build, SOURCE_DIR/foo/bar/baz.cpp files are processed by a special tool into BUILD_DIR/foo/bar/baz.hashed.cpp before being passed to the compiler (essentially, the tool replaces certain marked strings with an integer hash code of the string).

During a PGO build, the following happens:

1. The prof-gen build is done into PGO_TRAIN_BUILD_DIR. Thus, counters for *all* static functions end up with a name like PGO_TRAIN_BUILD_DIR/foo/bar/baz.hashed.cpp:funcname.
2. The prof-use build is done into PGO_BUILD_DIR. Thus, the counter name that is expected is PGO_BUILD_DIR/foo/bar/baz.hashed.cpp:funcname. Thus, the counter is not found. This affects *all* static functions.

Pre-inline mitigates the issue significantly (since static functions are likely to be inlined), but we still saw it occur in practice.

[jakev]

Added a flag for controlling the behavior of this. Keeping the name short but informative makes the flag name somewhat weird but just let me know if you have some preference on the naming.

[davidxl]

lgtm

[jakev]

Sean or David, could one of you please commit this for me, I don't have commit access. Thanks!

[davidxl]

Done: r275193.

David

llvm/trunk/lib/ProfileData/InstrProf.cpp

 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Path.h"
 
 using namespace llvm;
 
+static cl::opt<bool> StaticFuncFullModulePrefix(
+    "static-func-full-module-prefix", cl::init(false),
+    cl::desc("Use full module build paths in the profile counter names for "
+             "static functions."));
+
 namespace {
 std::string getInstrProfErrString(instrprof_error Err) {
   switch (Err) {
   case instrprof_error::success:
     return "Success";
   case instrprof_error::eof:
     return "End of File";
   case instrprof_error::unrecognized_format:

 // symbols. This happens after value profile annotation, but those internal
 // linkage functions should not have a source prefix.
 // To differentiate compiler generated internal symbols from original ones,
 // PGOFuncName meta data are created and attached to the original internal
 // symbols in the value profile annotation step
 // (PGOUseFunc::annotateIndirectCallSites). If a symbol does not have the meta
 // data, its original linkage must be non-internal.
 std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t Version) {
-  if (!InLTO)
-    return getPGOFuncName(F.getName(), F.getLinkage(), F.getParent()->getName(),
-                          Version);
+  if (!InLTO) {
+    StringRef FileName = (StaticFuncFullModulePrefix
+                              ? F.getParent()->getName()
+                              : sys::path::filename(F.getParent()->getName()));
+    return getPGOFuncName(F.getName(), F.getLinkage(), FileName, Version);
+  }
 
   // In LTO mode (when InLTO is true), first check if there is a meta data.
   if (MDNode *MD = getPGOFuncNameMetadata(F)) {
     StringRef S = cast<MDString>(MD->getOperand(0))->getString();
     return S.str();
   }
 
   // If there is no meta data, the function must be a global before the value

llvm/trunk/test/Transforms/PGOProfile/statics_counter_naming.ll

+; 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
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; GEN: @__profn_statics_counter_naming.ll_func = private constant [30 x i8] c"statics_counter_naming.ll:func"
+
+define internal i32 @func() {
+entry:
+  ret i32 0
+}