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

[xur]

Summary

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.

[tejohnson]

(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).

Slight correction - it doesn't affect ThinLTO static promotion (which uses the module hash not the guid). It affects the ThinLTO indirect call promotion since the PGO name needs to match the guid used in the index.

lib/IR/Globals.cpp

38 ↗

(On Diff #87009)

It isn't just profile counter names. "in the global identifier for static functions".

lib/ProfileData/InstrProf.cpp

31 ↗

(On Diff #87009)

Should this one be removed, is it useful with the new option?

[xur]

thanks to Teresa for the collection. I'll update the patch shortly

lib/IR/Globals.cpp

38 ↗

(On Diff #87009)

Fixed.

lib/ProfileData/InstrProf.cpp

31 ↗

(On Diff #87009)

This will be removed later as we assume this options is being used by the users.

[xur]

updated the option description per Teresa's suggestion.

[davidxl]

lib/IR/Globals.cpp

125 ↗

(On Diff #87021)

This is PGO specific, probably move the change into InstrProf.cpp including the option etc.

128 ↗

(On Diff #87021)

format fix.

159 ↗

(On Diff #87021)

getGlobalIdentifier is a low level interface. Move this into getPGOFuncName

test/Transforms/PGOProfile/statics_counter_naming.ll

4 ↗

(On Diff #87021)

The test does not actually test anything. Better add case that strips the expected separators.

[tejohnson]

lib/IR/Globals.cpp

159 ↗

(On Diff #87021)

Rong and I discussed this - putting it here ensures the same naming scheme is used for PGO and for the ThinLTO index. Although for ThinLTO we will likely not want this enabled. I'm not sure if there is a good way to enforce that.

[xur]

test/Transforms/PGOProfile/statics_counter_naming.ll

4 ↗

(On Diff #87021)

I can not assume the source directory structure. It's hard to write a test that strip the exact number of directory strings. But at least this test shows that the new option can replace the original option.

[davidxl]

test/Transforms/PGOProfile/statics_counter_naming.ll

4 ↗

(On Diff #87021)

ok. perhaps add a negative test that if stripping is not used, it will fail?

[silvas]

This change does two things (as you mention in the description):

1. Adding -static-func-strip-dirname-prefix which provides a way to have more control when -static-func-full-module-prefix=true is specified.
2. Changing the default of -static-func-full-module-prefix to true.

IIRC, -static-func-full-module-prefix defaults to false because it caused issues when set to true (in fact, it was introduced to avoid these issues). The default value of -static-func-strip-dirname-prefix introduced in this patch (i.e. 0) is effectively a no-op; so ignore 1. for now. This means that the net effect of this patch is that compilation will, by default, have a regression on the issue fixed by r275193 / http://reviews.llvm.org/D22028, which is not a good idea. I think that the default behavior (which is user-visible) should not be changed in this patch.

Overall, it sounds like this approach of relying on users to tweak internal compiler options (-mllvm) to get correct behavior in their environment is not the kind of user experience we want to deliver (or the kind of implementation that we want to maintain). IIRC, when we added -static-func-full-module-prefix, it was with the understanding that it was a simple hack for working around the larger issue of relying on the module name which we knew was not very robust. The further addition of the "InLTO" complicates things even further. It seems like a code smell that we do not have a Single Point Of Truth.

I proposed a solution at one point https://groups.google.com/d/msg/llvm-dev/s_VZbFTWbVs/d0b4Zh80CgAJ though it may no longer be applicable. It seems like ThinLTO already has to solve a problem of finding unique identifiers for all functions (even static), so we may want to piggy-back on that mechanism (this is just a high-level thought; haven't looked into the details).

So:

• I specifically object to changing user-visible defaults in this patch. Those changes should be isolated, and I don't think we have justification to change those defaults anyway.
• I'm slightly opposed to adding the -static-func-strip-dirname-prefix flag, since it seems like a workaround (among others that have already piled up) for a more fundamental issue. This is a frog-in-boiling-water situation; if solving the fundamental issue would be a huge amount of work, then adding the new flag is probably fine for now, but we need to keep in mind the larger situation. IIUC, defaulting -static-func-strip-dirname-prefix=-1 would emulate the current default behavior, so -static-func-full-module-prefix could just be removed in the same patch.
• I would encourage brainstorming/discussion of alternative solutions that solve the fundamental problem (which seems to be more about having a stable globally unique identifier than being specifically about preserving/mangling the "name" per se).

[mehdi_amini]

From the description:

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 patch turns the default value for -static-func-full-module-prefix to false.

However in the diff I see:

• "static-func-full-module-prefix", cl::init(false),

+ "static-func-full-module-prefix", cl::init(true),

Which is the opposite of the description? This seems confusing to me.

[xur]

Update the patch based on the reviews:
(1) move the code to ProfileData to make the change local to profile
(2) add negative test
(3) add a comment on how it affects thin-lto.

[xur]

In D29512#666652, @mehdi_amini wrote:

From the description:

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 patch turns the default value for -static-func-full-module-prefix to false.

However in the diff I see:

• "static-func-full-module-prefix", cl::init(false),

+ "static-func-full-module-prefix", cl::init(true),

Which is the opposite of the description? This seems confusing to me.

Sorry for the confusion. It seems my description was not accurate.

The current default value for -static-func-full-module-prefix is false, which means it strips all the path names.

This patch will make the default value as true, which keeps the path names.

[davidxl]

lgtm

llvm/trunk/lib/ProfileData/InstrProf.cpp

 #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),
+    "static-func-full-module-prefix", cl::init(true),
     cl::desc("Use full module build paths in the profile counter names for "
              "static functions."));
 
+// This option is tailored to users that have different top-level directory in
+// profile-gen and profile-use compilation. Users need to specific the number
+// of levels to strip. A value larger than the number of directories in the
+// source file will strip all the directory names and only leave the basename.
+//
+// Note current Thin-LTO module importing for the indirect-calls assumes
+// the source directory name not being stripped. A non-zero option value here
+// can potentially prevent some inter-module indirect-call-promotions.
+static cl::opt<unsigned> StaticFuncStripDirNamePrefix(
+    "static-func-strip-dirname-prefix", cl::init(0),
+    cl::desc("Strip specified level of directory name from source path in "
+             "the profile counter name 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:

 
 std::string getPGOFuncName(StringRef RawFuncName,
                            GlobalValue::LinkageTypes Linkage,
                            StringRef FileName,
                            uint64_t Version LLVM_ATTRIBUTE_UNUSED) {
   return GlobalValue::getGlobalIdentifier(RawFuncName, Linkage, FileName);
 }
 
+// Strip NumPrefix level of directory name from PathNameStr. If the number of
+// directory separators is less than 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);
+}
+
 // Return the PGOFuncName. This function has some special handling when called
 // in LTO optimization. The following only applies when calling in LTO passes
 // (when \c InLTO is true): LTO's internalization privatizes many global linkage
 // symbols. This happens after value profile annotation, but those internal
 // linkage functions should not have a source prefix.
 // Additionally, for ThinLTO mode, exported internal functions are promoted
 // and renamed. We need to ensure that the original internal PGO name is
 // used when computing the GUID that is compared against the profiled GUIDs.
 // 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) {
     StringRef FileName = (StaticFuncFullModulePrefix
                               ? F.getParent()->getName()
                               : sys::path::filename(F.getParent()->getName()));
+    if (StaticFuncFullModulePrefix && StaticFuncStripDirNamePrefix != 0)
+      FileName = stripDirPrefix(FileName, StaticFuncStripDirNamePrefix);
     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();
   }

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
+; RUN: opt %s -pgo-instr-gen -static-func-full-module-prefix=false -S | FileCheck %s --check-prefix=NOPATH
+; RUN: opt %s -passes=pgo-instr-gen -static-func-full-module-prefix=false -S | FileCheck %s --check-prefix=NOPATH
+; RUN: opt %s --pgo-instr-gen -static-func-strip-dirname-prefix=1000 -S | FileCheck %s --check-prefix=NOPATH
+; RUN: opt %s -passes=pgo-instr-gen -static-func-strip-dirname-prefix=1000 -S | FileCheck %s --check-prefix=NOPATH
+; RUN: opt %s --pgo-instr-gen -static-func-strip-dirname-prefix=1 -S | FileCheck %s --check-prefix=HASPATH
+; RUN: opt %s -passes=pgo-instr-gen -static-func-strip-dirname-prefix=1 -S | FileCheck %s --check-prefix=HASPATH
 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"
+; NOPATH: @__profn_statics_counter_naming.ll_func = private constant [30 x i8] c"statics_counter_naming.ll:func"
+; HASPATH-NOT: @__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
 }