Skip to content

[InstrProf] No linkage prefixes in IRPGO names #76994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,6 @@
// LTO if default linker is GNU ld or gold anyway.
// REQUIRES: lld-available

// Test should fail where linkage-name and mangled-name diverges, see issue https://github.com/llvm/llvm-project/issues/74565).
// Currently, this name divergence happens on Mach-O object file format, or on
// many (but not all) 32-bit Windows systems.
//
// XFAIL: system-darwin
//
// Mark 32-bit Windows as UNSUPPORTED for now as opposed to XFAIL. This test
// should fail on many (but not all) 32-bit Windows systems and succeed on the
// rest. The flexibility in triple string parsing makes it tricky to capture
// both sets accurately. i[3-9]86 specifies arch as Triple::ArchType::x86, (win32|windows)
// specifies OS as Triple::OS::Win32
//
// UNSUPPORTED: target={{i.86.*windows.*}}

// RUN: rm -rf %t && split-file %s %t && cd %t

// Do setup work for all below tests.
Expand Down
2 changes: 1 addition & 1 deletion lld/test/MachO/pgo-warn-mismatch.ll
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ entry:

;--- cs.proftext
:csir
_foo
foo
# Func Hash:
2277602155505015273
# Num Counters:
Expand Down
27 changes: 8 additions & 19 deletions llvm/lib/ProfileData/InstrProf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "llvm/ProfileData/InstrProf.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
Expand All @@ -27,7 +26,6 @@
#include "llvm/IR/Instruction.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
#include "llvm/IR/Mangler.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/Type.h"
Expand Down Expand Up @@ -297,29 +295,20 @@ static StringRef getStrippedSourceFileName(const GlobalObject &GO) {
return FileName;
}

// The PGO name has the format [<filepath>;]<linkage-name> where <filepath>; is
// provided if linkage is local and <linkage-name> is the mangled function
// name. The filepath is used to discriminate possibly identical function names.
// ; is used because it is unlikely to be found in either <filepath> or
// <linkage-name>.
// The PGO name has the format [<filepath>;]<mangled-name> where <filepath>; is
// provided if linkage is local and is used to discriminate possibly identical
// mangled names. ";" is used because it is unlikely to be found in either
// <filepath> or <mangled-name>.
//
// Older compilers used getPGOFuncName() which has the format
// [<filepath>:]<function-name>. <filepath> is used to discriminate between
// possibly identical function names when linkage is local and <function-name>
// simply comes from F.getName(). This caused trouble for Objective-C functions
// which commonly have :'s in their names. Also, since <function-name> is not
// mangled, they cannot be passed to Mach-O linkers via -order_file. We still
// need to compute this name to lookup functions from profiles built by older
// compilers.
// [<filepath>:]<mangled-name>. This caused trouble for Objective-C functions
// which commonly have :'s in their names. We still need to compute this name to
// lookup functions from profiles built by older compilers.
static std::string
getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
// FIXME: Mangler's handling is kept outside of `getGlobalIdentifier` for now.
// For more details please check issue #74565.
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we just keep getIRPGONameForGlobalObject method here and simply replace line 321 with another dummy helper which is a wrapper to getName() (with some comments)? This allows more platform specific handling flexibility in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return GlobalValue::getGlobalIdentifier(Name, Linkage, FileName);
return GlobalValue::getGlobalIdentifier(GO.getName(), Linkage, FileName);
}

static std::optional<std::string> lookupPGONameFromMetadata(MDNode *MD) {
Expand Down
6 changes: 5 additions & 1 deletion llvm/tools/llvm-profdata/llvm-profdata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3157,7 +3157,11 @@ static int order_main(int argc, const char *argv[]) {
BalancedPartitioning BP(Config);
BP.run(Nodes);

WithColor::note() << "# Ordered " << Nodes.size() << " functions\n";
OS << "# Ordered " << Nodes.size() << " functions\n";
OS << "# Warning: Mach-O may prefix symbols with \"_\" depending on the "
"linkage and this output does not take that into account. Some "
"post-processing may be required before passing to the linker via "
"-order_file.\n";
for (auto &N : Nodes) {
auto [Filename, ParsedFuncName] =
getParsedIRPGOFuncName(Reader->getSymtab().getFuncOrVarName(N.Id));
Expand Down
18 changes: 2 additions & 16 deletions llvm/unittests/ProfileData/InstrProfTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -542,22 +542,14 @@ TEST_F(InstrProfTest, test_memprof_merge) {
TEST_F(InstrProfTest, test_irpgo_function_name) {
LLVMContext Ctx;
auto M = std::make_unique<Module>("MyModule.cpp", Ctx);
// Use Mach-O mangling so that non-private symbols get a `_` prefix.
M->setDataLayout(DataLayout("m:o"));
auto *FTy = FunctionType::get(Type::getVoidTy(Ctx), /*isVarArg=*/false);

std::vector<std::tuple<StringRef, Function::LinkageTypes, StringRef>> Data;
Data.emplace_back("ExternalFoo", Function::ExternalLinkage, "_ExternalFoo");
Data.emplace_back("ExternalFoo", Function::ExternalLinkage, "ExternalFoo");
Data.emplace_back("InternalFoo", Function::InternalLinkage,
"MyModule.cpp;_InternalFoo");
Data.emplace_back("PrivateFoo", Function::PrivateLinkage,
"MyModule.cpp;l_PrivateFoo");
Data.emplace_back("WeakODRFoo", Function::WeakODRLinkage, "_WeakODRFoo");
// Test Objective-C symbols
"MyModule.cpp;InternalFoo");
Data.emplace_back("\01-[C dynamicFoo:]", Function::ExternalLinkage,
"-[C dynamicFoo:]");
Data.emplace_back("-<C directFoo:>", Function::ExternalLinkage,
"_-<C directFoo:>");
Data.emplace_back("\01-[C internalFoo:]", Function::InternalLinkage,
"MyModule.cpp;-[C internalFoo:]");

Expand Down Expand Up @@ -590,10 +582,6 @@ TEST_F(InstrProfTest, test_pgo_function_name) {
Data.emplace_back("ExternalFoo", Function::ExternalLinkage, "ExternalFoo");
Data.emplace_back("InternalFoo", Function::InternalLinkage,
"MyModule.cpp:InternalFoo");
Data.emplace_back("PrivateFoo", Function::PrivateLinkage,
"MyModule.cpp:PrivateFoo");
Data.emplace_back("WeakODRFoo", Function::WeakODRLinkage, "WeakODRFoo");
// Test Objective-C symbols
Data.emplace_back("\01-[C externalFoo:]", Function::ExternalLinkage,
"-[C externalFoo:]");
Data.emplace_back("\01-[C internalFoo:]", Function::InternalLinkage,
Expand All @@ -611,8 +599,6 @@ TEST_F(InstrProfTest, test_pgo_function_name) {
TEST_F(InstrProfTest, test_irpgo_read_deprecated_names) {
LLVMContext Ctx;
auto M = std::make_unique<Module>("MyModule.cpp", Ctx);
// Use Mach-O mangling so that non-private symbols get a `_` prefix.
M->setDataLayout(DataLayout("m:o"));
auto *FTy = FunctionType::get(Type::getVoidTy(Ctx), /*isVarArg=*/false);
auto *InternalFooF =
Function::Create(FTy, Function::InternalLinkage, "InternalFoo", M.get());
Expand Down