Prepend "uniq" to symbol names hash with -funique-internal-linkage-names

[tmsriram]

Summary

TLDR; Prepend the module name hash with a fixed string ".__uniq." which helps tools that consume sampled profiles and attribute it to functions to understand that this symbol belongs to a unique internal linkage type symbol.

Symbols with suffixes can result from various optimizations in the compiler. Function Multiversioning, function splitting, parameter constant propogation, unique internal linkage names.

External tools like sampled profile aggregators combine profiles from multiple runs of a binary. They use various heuristics with symbols that have suffixes to try and attribute the profile to the right function instance. For instance multi-versioned symbols like foo.avx, foo.sse4.2, etc even though different should be attributed to the same source function if a single function is versioned, using attribute target_clones (supported in GCC but yet to land in LLVM). Similarly, functions that are split (split part having a .cold suffix) could have profiles for both the original and split symbols but would be aggregated and attributed to the original function that was split.

Unique internal linkage functions however have different source instances and the aggregator must not put them together but attribute it to the appropriate function instance. To be sure that we are dealing with a symbol of a unique internal linkage function, we would like to prepend the hash with a known string ".__uniq." which these tools can check to understand the suffix type.

[mtrofin]

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

32

would .uniq. (so a dot before and after) fit more with existing other cases (like .llvm.)?

[tmsriram]

Prepend ".uniq." to the module name hash.

As mtrofin@ observerd, ".llvm." is prepended to the suffix of global names that are promoted from local in ModuleSummaryIndex and this is consistent with that.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

32

Thanks, .llvm. is indeed used when creating a promoted global name for a local with its original module ID. So, .uniq. would be consistent with that.

[tmsriram]

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

[mtrofin]

In D89617#2339557, @tmsriram wrote:

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

Unique-ing names this way isn't, at core, something that's too tied to llvm, even if we implement it here first. There are tools that would leverage this feature by binding to the suffix. With a look at other present or future compilers, and given that making this decision now is free, and much more expensive later, I think it would be beneficial if we were intentional about:

• not using "llvm" to indicate this is something tool-generated, and instead use, say "" (".uniq.")

or

• using "llvm", on the grounds that there are other examples of general-purpose concepts that have tool-specific prefixes (like gcc_except_table, that is supported in llvm); so the prefixed name is basically a proper noun, it doesn't mean "llvm only" or "gcc only".

I believe either way, the ".llvm." case is going to stick out as an inconsistency: it should have been either ".llvmlto." or "__lto", in either case, I don't think it serves too well as an argument for the llvm prefix.

[tmsriram]

In D89617#2339607, @mtrofin wrote:

In D89617#2339557, @tmsriram wrote:

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

Unique-ing names this way isn't, at core, something that's too tied to llvm, even if we implement it here first. There are tools that would leverage this feature by binding to the suffix. With a look at other present or future compilers, and given that making this decision now is free, and much more expensive later, I think it would be beneficial if we were intentional about:

• not using "llvm" to indicate this is something tool-generated, and instead use, say "" (".uniq.")

or

• using "llvm", on the grounds that there are other examples of general-purpose concepts that have tool-specific prefixes (like gcc_except_table, that is supported in llvm); so the prefixed name is basically a proper noun, it doesn't mean "llvm only" or "gcc only".

I believe either way, the ".llvm." case is going to stick out as an inconsistency: it should have been either ".llvmlto." or "__lto", in either case, I don't think it serves too well as an argument for the llvm prefix.

Acknowledged and great points. I just want to add that we should pick something that would minimize collisions with future ISA names that would land as suffixes with multiversion symbols.

[mtrofin]

In D89617#2347705, @tmsriram wrote:

In D89617#2339607, @mtrofin wrote:

In D89617#2339557, @tmsriram wrote:

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

Unique-ing names this way isn't, at core, something that's too tied to llvm, even if we implement it here first. There are tools that would leverage this feature by binding to the suffix. With a look at other present or future compilers, and given that making this decision now is free, and much more expensive later, I think it would be beneficial if we were intentional about:

• not using "llvm" to indicate this is something tool-generated, and instead use, say "" (".uniq.")

or

• using "llvm", on the grounds that there are other examples of general-purpose concepts that have tool-specific prefixes (like gcc_except_table, that is supported in llvm); so the prefixed name is basically a proper noun, it doesn't mean "llvm only" or "gcc only".

I believe either way, the ".llvm." case is going to stick out as an inconsistency: it should have been either ".llvmlto." or "__lto", in either case, I don't think it serves too well as an argument for the llvm prefix.

Acknowledged and great points. I just want to add that we should pick something that would minimize collisions with future ISA names that would land as suffixes with multiversion symbols.

Ack - I'd propose "__" then, it very likely won't collide, it's compiler-independent, and (arguably) suggests tooling as origin.

[tmsriram]

Change the prefix to "__uniq" like mtrofin@ suggested.

[mtrofin]

lgtm

[MaskRay]

This comment was removed by MaskRay.

clang/test/CodeGen/unique-internal-linkage-names.cpp

 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
-// UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
-// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
-// UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
+// UNIQUE: @_ZL4glob.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9a-f]+}} = internal global
+// UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9a-f]+}}
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
-// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
-// UNIQUEO1: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
+// UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
 // UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
-// UNIQUEO1: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
-// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
+// UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
+// UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

 
 static bool uniqueifyInternalLinkageNames(Module &M) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
   llvm::MD5::MD5Result R;
   Md5.final(R);
   SmallString<32> Str;
   llvm::MD5::stringifyResult(R, Str);
-  std::string ModuleNameHash = (Twine(".") + Twine(Str)).str();
+  // Prepend "__uniq" before the hash for tools like profilers to understand that
+  // this symbol is of internal linkage type.
+  std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
 
   // Append the module hash to all internal linkage functions.
   for (auto &F : M) {
     if (F.hasInternalLinkage()) {
       F.setName(F.getName() + ModuleNameHash);
       Changed = true;
     }

llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll

 ; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s
 
 source_filename = "foo.c"
 
 @glob = internal global i32 0
 
 define internal i32 @foo() {
 entry:
   ret i32 0
 }
 
-; CHECK: @glob.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
-; CHECK: define internal i32 @foo.6ae72bb15a7d1834b42ae042a58f7a4d()
+; CHECK: @glob.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d = internal global
+; CHECK: define internal i32 @foo.__uniq.6ae72bb15a7d1834b42ae042a58f7a4d()