This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wmi on Mar 10 2021, 8:47 PM.

Details

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.

Diff Detail

Event Timeline

wmi created this revision.Mar 10 2021, 8:47 PM
wmi requested review of this revision.Mar 10 2021, 8:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 8:47 PM
tmsriram added inline comments.Mar 10 2021, 8:51 PM
llvm/lib/ProfileData/InstrProf.cpp
359

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 added inline comments.Mar 10 2021, 8:52 PM
llvm/lib/ProfileData/InstrProf.cpp
359

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

wmi added inline comments.Mar 10 2021, 9:23 PM
llvm/lib/ProfileData/InstrProf.cpp
359

'.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 added inline comments.Mar 10 2021, 9:33 PM
llvm/lib/ProfileData/InstrProf.cpp
359

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

wmi added inline comments.Mar 10 2021, 9:42 PM
llvm/lib/ProfileData/InstrProf.cpp
359

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 updated this revision to Diff 329851.Mar 10 2021, 11:00 PM

Address reviewers' comments.

hoy accepted this revision.Mar 11 2021, 10:10 AM

LGTM, thanks.

This revision is now accepted and ready to land.Mar 11 2021, 10:10 AM
wenlei accepted this revision.Mar 11 2021, 10:11 AM

lgtm.

tmsriram accepted this revision.Mar 11 2021, 10:33 AM
This revision was landed with ongoing or failed builds.Mar 11 2021, 11:09 AM
This revision was automatically updated to reflect the committed changes.

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 added a comment.Mar 11 2021, 2:58 PM

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.

In D98389#2620729, @wmi 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 added a comment.Mar 12 2021, 1:55 PM
In D98389#2620729, @wmi 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