[IndirectCallPromotion] Don't strip ".__uniq." suffix when it strips ".llvm." suffix.

[wmi]

Summary

Currently IndirectCallPromotion simply strip everything after the first "." in LTO mode, in order to match the symbol name and the name with ".llvm." suffix in the value profile. However, if -funique-internal-linkage-names and thinlto are both enabled, the name may have both ".uniq." suffix and ".llvm." suffix (".uniq." will only exist before ".llvm."), and the current mechanism will strip them both, which will block indirect call promotion in both InstrPGO and SamplePGO modes. The patch fixes the problem.

[tmsriram]

llvm/lib/ProfileData/InstrProf.cpp

355–356

Not too familiar with this code, is there a chance you could have a different suffix other than "__uniq" before the .llvm suffix like say "constprop" ? In this case that suffix would be retained by this change which would be different from the original behavior.

[wenlei]

llvm/lib/ProfileData/InstrProf.cpp

355–356

getCanonicalFnName has other known suffixes, do we need to consider these as well?

[wmi]

llvm/lib/ProfileData/InstrProf.cpp

355–356

'.constprop.' and '.part.' suffix handled in getCanonicalFnName were used by GCC but I don't find them in llvm. I am not aware of function suffix other than ".llvm." and ".uniq." used by llvm, but you and Sri are right it is good to keep the original behavior as much as possible. I will change it to only keep ".uniq.".

[hoy]

llvm/lib/ProfileData/InstrProf.cpp

355–356

Wondering if it makes sense to use getCanonicalFnName here, maybe with an argument indicating only ".uniq" should be kept.

[wmi]

llvm/lib/ProfileData/InstrProf.cpp

355–356

Here instrPGO is also impacted. To make instrPGO and samplePGO share getCanonicalFnName means a larger churn. I will put an argument there to make it clear.

[wmi]

Address reviewers' comments.

[hoy]

LGTM, thanks.

[wenlei]

lgtm.

[mstorsjo]

FWIW, I'm seeing errors like "Malformed instrumentation profile data" when building code for a i686-w64-mingw32 target after this commit. I've yet to reduce the actual trigger for it, but I thought I'd let you know...

[wmi]

In D98389#2620644, @mstorsjo wrote:

FWIW, I'm seeing errors like "Malformed instrumentation profile data" when building code for a i686-w64-mingw32 target after this commit. I've yet to reduce the actual trigger for it, but I thought I'd let you know...

Thanks Martin for reporting the error. I think the error is thrown in the call of addFuncName(OtherFuncName) here:
https://github.com/llvm/llvm-project/blob/b552adf8b388a4fbdaa6fb46bdedc83fc738fc2b/llvm/lib/ProfileData/InstrProf.cpp#L376

Could you help to get the "PGOFuncName" string before addFuncName throws the error? Then we can know what is wrong.

[mstorsjo]

In D98389#2620729, @wmi wrote:

In D98389#2620644, @mstorsjo wrote:

FWIW, I'm seeing errors like "Malformed instrumentation profile data" when building code for a i686-w64-mingw32 target after this commit. I've yet to reduce the actual trigger for it, but I thought I'd let you know...

Thanks Martin for reporting the error. I think the error is thrown in the call of addFuncName(OtherFuncName) here:
https://github.com/llvm/llvm-project/blob/b552adf8b388a4fbdaa6fb46bdedc83fc738fc2b/llvm/lib/ProfileData/InstrProf.cpp#L376

Could you help to get the "PGOFuncName" string before addFuncName throws the error? Then we can know what is wrong.

Ok, sure. Here's a full reproducer:

$ cat ../repro.c 
static void func(void) { }
void (*funcptr)(void) = func;
$ clang -target x86_64-linux-gnu -c ../repro.c -O2
PGOFuncName "../repro.c:func"
OtherFuncName ""
Program aborted due to an unhandled Error:
Malformed instrumentation profile data

[wmi]

In D98389#2621398, @mstorsjo wrote:

In D98389#2620729, @wmi wrote:

In D98389#2620644, @mstorsjo wrote:

FWIW, I'm seeing errors like "Malformed instrumentation profile data" when building code for a i686-w64-mingw32 target after this commit. I've yet to reduce the actual trigger for it, but I thought I'd let you know...

Thanks Martin for reporting the error. I think the error is thrown in the call of addFuncName(OtherFuncName) here:
https://github.com/llvm/llvm-project/blob/b552adf8b388a4fbdaa6fb46bdedc83fc738fc2b/llvm/lib/ProfileData/InstrProf.cpp#L376

Could you help to get the "PGOFuncName" string before addFuncName throws the error? Then we can know what is wrong.

Ok, sure. Here's a full reproducer:

$ cat ../repro.c 
static void func(void) { }
void (*funcptr)(void) = func;
$ clang -target x86_64-linux-gnu -c ../repro.c -O2
PGOFuncName "../repro.c:func"
OtherFuncName ""
Program aborted due to an unhandled Error:
Malformed instrumentation profile data

Thanks Martin. The problem is fixed and patch recommitted at https://github.com/llvm/llvm-project/commit/ef9d7db72362b2df1237a87d5dc7dad6b0105df6

llvm/lib/ProfileData/InstrProf.cpp

for (Function &F : M) {
     // Function may not have a name: like using asm("") to overwrite the name.
     // Ignore in this case.
     if (!F.hasName())
       continue;
     const std::string &PGOFuncName = getPGOFuncName(F, InLTO);
     if (Error E = addFuncName(PGOFuncName))
       return E;
     MD5FuncMap.emplace_back(Function::getGUID(PGOFuncName), &F);
     // In ThinLTO, local function may have been promoted to global and have
-    // suffix added to the function name. We need to add the stripped function
-    // name to the symbol table so that we can find a match from profile.
-    if (InLTO) {
-      auto pos = PGOFuncName.find('.');
-      if (pos != std::string::npos) {
-        const std::string &OtherFuncName = PGOFuncName.substr(0, pos);
-        if (Error E = addFuncName(OtherFuncName))
-          return E;
-        MD5FuncMap.emplace_back(Function::getGUID(OtherFuncName), &F);
-      }
-    }
-  }
+    // suffix ".llvm." added to the function name. We need to add the
+    // stripped function name to the symbol table so that we can find a match
+    // from profile.
+    //
+    // We may have other suffixes similar as ".llvm." which are needed to
+    // be stripped before the matching, but ".__uniq." suffix which is used
+    // to differentiate internal linkage functions in different modules
+    // should be kept. Now this is the only suffix with the pattern ".xxx"
+    // which is kept before matching.
+    const std::string UniqSuffix = ".__uniq.";
+    auto pos = PGOFuncName.find(UniqSuffix);
+    // Search '.' after ".__uniq." if ".__uniq." exists, otherwise
+    // search '.' from the beginning.
+    if (pos != std::string::npos)
+      pos += UniqSuffix.length();
+    else
+      pos = 0;
+    pos = PGOFuncName.find('.', pos);
   Sorted = false;
   finalizeSymtab();
   return Error::success();
 }
 
 uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) {
   finalizeSymtab();
   auto It = partition_point(AddrToMD5Map, [=](std::pair<uint64_t, uint64_t> A) {

llvm/test/Transforms/PGOProfile/indirect_call_promotion_unique.ll

+; RUN: opt < %s -passes=pgo-icall-prom -S | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@foo = common global i32 ()* null, align 8
+
+; The names on the IR and in the profile are both "func1".
+define i32 @func1() {
+entry:
+  ret i32 1
+}
+
+define i32 @bar1() {
+entry:
+  %tmp1 = load i32 ()*, i32 ()** @foo, align 8
+; CHECK: icmp eq i32 ()* %tmp1, @func1
+  %call = call i32 %tmp1(), !prof !1
+  ret i32 %call
+}
+
+; The name on the IR has ".llvm." suffix: "func2.llvm.10895321227755557127".
+; The name in the profile has no ".llvm." suffix: "func2".
+define i32 @func2.llvm.10895321227755557127() {
+entry:
+  ret i32 1
+}
+
+define i32 @bar2() {
+entry:
+  %tmp2 = load i32 ()*, i32 ()** @foo, align 8
+; CHECK: icmp eq i32 ()* %tmp2, @func2.llvm.10895321227755557127
+  %call = call i32 %tmp2(), !prof !2
+  ret i32 %call
+}
+
+; The names on the IR and in the profile are both
+; "func3.__uniq.258901567653530696343884446915951489119".
+define i32 @func3.__uniq.258901567653530696343884446915951489119() {
+entry:
+  ret i32 2
+}
+
+define i32 @bar3() {
+entry:
+  %tmp3 = load i32 ()*, i32 ()** @foo, align 8
+; CHECK: icmp eq i32 ()* %tmp3, @func3.__uniq.258901567653530696343884446915951489119
+  %call = call i32 %tmp3(), !prof !3
+  ret i32 %call
+}
+
+; The name on the IR has ".__uniq." and ".llvm." suffix:
+; "func4.__uniq.140291095734751150107370763113257199296.llvm.10650195578168450516".
+; The name in the profile has ".__uniq." but no ".llvm." suffix:
+; "func4.__uniq.140291095734751150107370763113257199296".
+define i32 @func4.__uniq.140291095734751150107370763113257199296.llvm.10650195578168450516() {
+entry:
+  ret i32 3
+}
+
+define i32 @bar4() {
+entry:
+  %tmp4 = load i32 ()*, i32 ()** @foo, align 8
+; CHECK: icmp eq i32 ()* %tmp4, @func4.__uniq.140291095734751150107370763113257199296.llvm.10650195578168450516
+  %call = call i32 %tmp4(), !prof !4
+  ret i32 %call
+}
+
+; The name on the IR has ".__uniq.", ".part." and ".llvm." suffix:
+; "func4.__uniq.127882361580787111523790444488985774976.part.818292359123831.llvm.10650195578168450516".
+; The name in the profile has ".__uniq." but no ".llvm." and no ".part." suffix:
+; "func4.__uniq.127882361580787111523790444488985774976".
+define i32 @func5.__uniq.127882361580787111523790444488985774976.part.818292359123831.llvm.10650195578168450516() {
+entry:
+  ret i32 3
+}
+
+define i32 @bar5() {
+entry:
+  %tmp5 = load i32 ()*, i32 ()** @foo, align 8
+; CHECK: icmp eq i32 ()* %tmp5, @func5.__uniq.127882361580787111523790444488985774976.part.818292359123831.llvm.10650195578168450516
+  %call = call i32 %tmp5(), !prof !5
+  ret i32 %call
+}
+
+; GUID of "func1" is -2545542355363006406.
+; GUID of "func2" is -4377547752858689819.
+; GUID of "func3.__uniq.258901567653530696343884446915951489119" is 8271224222042874235.
+; GUID of "func4.__uniq.140291095734751150107370763113257199296" is 1491826207425861106.
+; GUID of "func5.__uniq.127882361580787111523790444488985774976" is -4238550483433487304.
+!1 = !{!"VP", i32 0, i64 1600, i64 -2545542355363006406, i64 1600}
+!2 = !{!"VP", i32 0, i64 1600, i64 -4377547752858689819, i64 1600}
+!3 = !{!"VP", i32 0, i64 1600, i64 8271224222042874235, i64 1600}
+!4 = !{!"VP", i32 0, i64 1600, i64 1491826207425861106, i64 1600}
+!5 = !{!"VP", i32 0, i64 1600, i64 -4238550483433487304, i64 1600}