[clang-tools-extra] [compiler-rt] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
minglotus-6 wrote:
> Resolved comments except the pending discussion about whether to use
> `.proftext` or`.profraw`.
>
> Added `REQUIRES: zlib` for LLVM IR test since the profile reader should be
> built with zlib support.
>
> I'll probably spend sometime to get this test running on my laptop (haven't
> tried to build llvm on mac before), while waiting for more feedbacks. I'm
> thinking of submitting it on Thursday or Friday. @ellishg I think the added
> compiler-rt test (on macosx) should be a test case for issue 74565.
The test failed on mac, but initially on trying to find `dso_local` of line
`PGOName: define dso_local void @_Z7callee1v() {{.*}} !prof ![[#]] !PGOFuncName
![[#]] `.
- Debugging with `lldb` points to [this
comment](https://github.com/llvm/llvm-project/blob/4f1ddf7523c0bbb4075b1682dbe2278080642eee/clang/lib/CodeGen/CodeGenModule.cpp#L1528),
saying only COFF and ELF assumes `dso_local`.
After making the `dso_local` optional in the [latest
commit](https://github.com/llvm/llvm-project/pull/74008/commits/f6a718d80d7a70d60ef8affc282f525e8282),
the test started to fail due to the difference of `` and
``.
1. Test failed, when external func `callee1` in expected output doesn't have
`!PGOFuncName` metadata but the test output has it. Note `!PGOFuncName` is
annotated [only
if](https://github.com/llvm/llvm-project/blob/90c23981768713736208d76578ca119a20f6ac60/llvm/lib/ProfileData/InstrProf.cpp#L1262-1265)
`PGOName != Func.getName()`
2. Fixing the first error allows me to see the missed import errors.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang-tools-extra] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -0,0 +1,115 @@
+// This is a regression test for ThinLTO indirect-call-promotion when candidate
+// callees need to be imported from another IR module. In the C++ test case,
+// `main` calls `global_func` which is defined in another module. `global_func`
+// has two indirect callees, one has external linkage and one has local
linkage.
+// All three functions should be imported into the IR module of main.
+
+// What the test does:
+// - Generate raw profiles from executables and convert it to indexed profiles.
+// During the conversion, a profiled callee address in raw profiles will be
+// converted to function hash in indexed profiles.
+// - Run IRPGO profile use and ThinTLO prelink pipeline and get LLVM bitcodes
+// for both cpp files in the C++ test case.
+// - Generate ThinLTO summary file with LLVM bitcodes, and run
`function-import` pass.
+// - Run `pgo-icall-prom` pass for the IR module which needs to import callees.
+
+// Use lld as linker for more robust test. We need to REQUIRE LLVMgold.so for
+// 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[3-9]86-.*-(win32|windows).*}}
+
+// RUN: rm -rf %t && split-file %s %t && cd %t
+
+// Do setup work for all below tests.
+// Generate raw profiles from real programs and convert it into indexed
profiles.
+// Use clangxx_pgogen for IR level instrumentation for C++.
+// RUN: %clangxx_pgogen -fuse-ld=lld -O2 lib.cpp main.cpp -o main
+// RUN: env LLVM_PROFILE_FILE=main.profraw %run ./main
+// RUN: llvm-profdata merge main.profraw -o main.profdata
+
+// Use profile on lib and get bitcode, test that local function callee0 has
+// expected !PGOFuncName metadata and external function callee1 doesn't have
+// !PGOFuncName metadata. Explicitly skip ICP pass to test ICP happens as
+// expected in the IR module that imports functions from lib.
+// RUN: %clang -mllvm -disable-icp -fprofile-use=main.profdata -flto=thin -O2
-c lib.cpp -o lib.bc
+// RUN: llvm-dis lib.bc -o - | FileCheck %s --check-prefix=PGOName
+
+// Use profile on main and get bitcode.
+// RUN: %clang -fprofile-use=main.profdata -flto=thin -O2 -c main.cpp -o
main.bc
+
+// Run llvm-lto to get summary file.
+// RUN: llvm-lto -thinlto -o summary main.bc lib.bc
+
+// Test the imports of functions. Default import thresholds would work but do
+// explicit override to be more futureproof. Note all functions have one basic
+// block with a function-entry-count of one, so they are actually hot functions
+// per default profile summary hotness cutoff.
+// RUN: opt -passes=function-import -import-instr-limit=100
-import-cold-multiplier=1 -summary-file summary.thinlto.bc main.bc -o
main.import.bc -print-imports 2>&1 | FileCheck %s --check-prefix=IMPORTS
+// Test that '_Z11global_funcv' has indirect calls annotated with value
profiles.
+// RUN: llvm-dis main.import.bc -o - | FileCheck %s --check-prefix=IR
+
+// Test that both candidates are ICP'ed and there is no `!VP` in the IR.
+// RUN: opt main.import.bc -icp-lto -passes=pgo-icall-prom -S
-pass-remarks=pgo-icall-prom 2>&1 | FileCheck %s
--check-prefixes=ICP-IR,ICP-REMARK --implicit-check-not="!VP"
+
+// IMPORTS: main.cpp: Import _Z7callee1v
+// IMPORTS: main.cpp: Import _ZL7callee0v.llvm.[[#]]
+// IMPORTS: main.cpp: Import _Z11global_funcv
+
+// PGOName: define dso_local void @_Z7callee1v() {{.*}} !prof ![[#]] {
+// PGOName: define internal void @_ZL7callee0v() {{.*}} !prof ![[#]]
!PGOFuncName ![[#MD:]] {
+// PGOName: ![[#MD]] = !{!"{{.*}}lib.cpp;_ZL7callee0v"}
+
+// IR-LABEL: define available_externally {{.*}} void @_Z11global_funcv()
{{.*}} !prof ![[#]] {
+// IR-NEXT: entry:
+// IR-NEXT: %0 = load ptr, ptr @calleeAddrs
+// IR-NEXT: tail call void %0(), !prof ![[#PROF1:]]
+// IR-NEXT: %1 = load ptr, ptr getelementptr inbounds ([2 x ptr], ptr
@calleeAddrs,
+// IR-NEXT: tail call void %1(), !prof ![[#PROF2:]]
+
+// The GUID of indirect callee is the MD5 hash of
`/path/to/lib.cpp:_ZL7callee0v`
teresajohnson wrote:
should this have the semicolon separator not a colon?
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang-tools-extra] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -1,39 +0,0 @@ -; Do setup work for all below tests: generate bitcode and combined index teresajohnson wrote: Without use of the raw profile, this test would not have caught the regression. If we think the new compiler-rt test is enough to catch this case in the future, then perhaps we don't need this LLVM test at all (there should already be some ICP tests). But if the compiler-rt test is not enough, because some people don't run those, then this should stay and use a raw profile as input. https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [compiler-rt] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
https://github.com/MaskRay edited https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang-tools-extra] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
minglotus-6 wrote: > . For IR PGO, there is basically no need to do so as the instrumentation and > profile-use should be in-sync. For front-end instrumentation, there seem to > be some use cases to use out of sync profile: https://reviews.llvm.org/D51240. Thanks for double checking. I noticed the ICP and stale profile tolerance discussions when read the Phab history; it's good Phab review history are still available nowadays. IRPGO profiles could be used along with supplementary sample-pgo profiles. I'll probably read relevant code in llvm-profdata to understand how these interact in theory mostly for my own curiosity (hopefully no rough edges as long as `llvm-profdata` uses the same pgo name format used by latest compiler) https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang-tools-extra] [clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -1,39 +1,45 @@ -; Do setup work for all below tests: generate bitcode and combined index -; RUN: opt -module-summary %s -o %t.bc -; RUN: opt -module-summary %p/Inputs/thinlto_indirect_call_promotion.ll -o %t2.bc +; The raw profiles and reduced IR inputs are generated from Inputs/update_icall_promotion_inputs.sh teresajohnson wrote: No because the point of this test is to catch regressions if the separator character (which is hardcoded in the proftext) changes again in the future. https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
minglotus-6 wrote: Just noticed there is a merge conflict. Will update my fork and merge. https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
https://github.com/minglotus-6 edited https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
minglotus-6 wrote:
> I think it makes more sense to use linkage-names for IRPGO, -order_file, and
> ThinLTO. -order_file is used in the linker when it only knows linkage-names,
> so I don't think it makes sense to feed it mangled names.
Thanks for the input. (not to bikeshed but) I think for the purpose of
computing global identifier, unique names should suffice. Clang-FE mangled
names are unique so sounds fine.
Using linkage-name would be a non-trivial change, given the static
`getGlobalIdentifier` takes a stringified name currently, and using
linkage-name means requiring compatible change in each callsite (e.g., if the
caller context doesn't have `GlobalValue` but just stringified names in the
bitcode, make sure linkage-name exists in the bitcode,
[this](https://github.com/llvm/llvm-project/blob/d6fbd96e5eaf3e8acbf1b43dce7a311352907567/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L6763)
seems one example). An alternative fixup is to store the MD5 of
`[filename;]linkage-name` (this is what's currently stored in this
[field](https://github.com/llvm/llvm-project/blob/0f45e45847a5f2969b8021c787a566531fc96473/compiler-rt/include/profile/InstrProfData.inc#L72-L74))
and the MD5 of `[filename:]mangled-name` (the original hash before D156569) in
the raw profiles, so `llvm-profdata` could choose properly (former for ordering
and latter for ICP)
Would you mind if I create a Github issue to track how to fix other potential
cases and assign it to you ? This PR would solve the colon and semicolon
difference.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
minglotus-6 wrote:
To avoid subtle issues when linkage-name is different from mangled names,,I'm
wondering if it warrants a change to use linkage-names (as opposed to mangled
name) in `GlobalValue::getGlobalIdentifier` in this PR. Global identifier is
supposed to be hash of unique names, and linkage-name is already unique.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
https://github.com/minglotus-6 updated
https://github.com/llvm/llvm-project/pull/74008
>From 4cb5b087485124a7f2375fdc018b42a0401e6409 Mon Sep 17 00:00:00 2001
From: mingmingl
Date: Thu, 30 Nov 2023 15:41:37 -0800
Subject: [PATCH 1/3] [PGO][GlobalValue][LTO]In
GlobalValues::getGlobalIdentifier, use semicolon as delimiter for
local-linkage varibles.
Commit fe05193 (phab D156569), IRPGO names uses format
'[;]' while prior format is
[:'. The format change would break the use caes
demonstrated in (updated)
llvm/test/Transforms/PGOProfile/thinlto_indirect_call_promotion.ll
This patch changes GlobalValues::getGlobalIdentifer to use the
semicolon.
To elaborate on the scenario how things break without this PR
1. IRPGO raw profiles stores (compressed) IRPGO names of functions in
one section, and per-function profile data in another section. One
field in per-function profile data is the MD5 hash of IRPGO names.
2. When raw profiles are converted to indexed format profiles, the
profiled address is mapped to the MD5 hash of the callee.
3. In thin-lto prelink pipeline, MD5 hash of IRPGO names will be
annotated as value profiles, and used to import indirect-call-prom
candidates. If the annotated MD5 hash is computed from the new format
while import uses the prior format, the callee cannot be imported.
The updated test case Transforms/PGOProfile/thinlto_indirect_call_promotion.ll
exercise the following path
- Annotate raw profiles and generate import summaries. Using the
imported summaries, it tests that functions are correctly imported and
ICP transformations happened.
---
llvm/lib/IR/Globals.cpp | 4 +-
llvm/lib/ProfileData/InstrProf.cpp| 28 +--
.../thinlto-function-summary-originalnames.ll | 6 +-
llvm/test/ThinLTO/X86/memprof-basic.ll| 26 +++
.../X86/memprof-duplicate-context-ids.ll | 12 +--
.../ThinLTO/X86/memprof-funcassigncloning.ll | 6 +-
llvm/test/ThinLTO/X86/memprof-indirectcall.ll | 32
llvm/test/ThinLTO/X86/memprof-inlined.ll | 14 ++--
.../Inputs/thinlto_icall_prom.profdata| Bin 0 -> 976 bytes
.../Inputs/thinlto_indirect_call_promotion.ll | 32 ++--
.../Inputs/update_icall_promotion_inputs.sh | 70 ++
.../thinlto_indirect_call_promotion.ll| 52 ++---
12 files changed, 194 insertions(+), 88 deletions(-)
create mode 100644
llvm/test/Transforms/PGOProfile/Inputs/thinlto_icall_prom.profdata
create mode 100644
llvm/test/Transforms/PGOProfile/Inputs/update_icall_promotion_inputs.sh
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
index 7bd4503a689e4..e821de3b198f1 100644
--- a/llvm/lib/IR/Globals.cpp
+++ b/llvm/lib/IR/Globals.cpp
@@ -158,9 +158,9 @@ std::string GlobalValue::getGlobalIdentifier(StringRef Name,
// that it will stay the same, e.g., if the files are checked out from
// version control in different locations.
if (FileName.empty())
- NewName = NewName.insert(0, ":");
+ NewName = NewName.insert(0, ";");
else
- NewName = NewName.insert(0, FileName.str() + ":");
+ NewName = NewName.insert(0, FileName.str() + ";");
}
return NewName;
}
diff --git a/llvm/lib/ProfileData/InstrProf.cpp
b/llvm/lib/ProfileData/InstrProf.cpp
index 236b083a1e215..d9ad5c8b6f683 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -246,11 +246,27 @@ std::string InstrProfError::message() const {
char InstrProfError::ID = 0;
-std::string getPGOFuncName(StringRef RawFuncName,
- GlobalValue::LinkageTypes Linkage,
+std::string getPGOFuncName(StringRef Name, GlobalValue::LinkageTypes Linkage,
StringRef FileName,
uint64_t Version LLVM_ATTRIBUTE_UNUSED) {
- return GlobalValue::getGlobalIdentifier(RawFuncName, Linkage, FileName);
+ // Value names may be prefixed with a binary '1' to indicate
+ // that the backend should not modify the symbols due to any platform
+ // naming convention. Do not include that '1' in the PGO profile name.
+ if (Name[0] == '\1')
+Name = Name.substr(1);
+
+ std::string NewName = std::string(Name);
+ if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
+// For local symbols, prepend the main file name to distinguish them.
+// Do not include the full path in the file name since there's no guarantee
+// that it will stay the same, e.g., if the files are checked out from
+// version control in different locations.
+if (FileName.empty())
+ NewName = NewName.insert(0, ":");
+else
+ NewName = NewName.insert(0, FileName.str() + ":");
+ }
+ return NewName;
}
// Strip NumPrefix level of directory name from PathNameStr. If the number of
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
S
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
ellishg wrote:
> Was PGO broken in general before D156569? Or is this specific to
> -symbol-ordering-file and -order_file?
It wasn't broken in general, but it's needed to get `-order_file` working
correctly.
> Also, it looks like some of the main changes here by the mangler for
> objective C are to strip the "\01" prefix. Is this different than the support
> to remove the '\1' mangling escape in getGlobalIdentifier()?
I think both remove the `\01` prefix. But the mangler might also prepend
`_`/`l_` depending on the linkage, and `getGlobalIdentifier()` does not do this.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
ellishg wrote:
I think we can only generate the linkage name at compile time because we need
to know the linkage, which `llvm-profdata` doesn't have.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
ellishg wrote:
Well, I guess the name is still mangled, but I used
`Mangler().getNameWithPrefix()` to get the linkage name.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -246,11 +246,27 @@ std::string InstrProfError::message() const {
char InstrProfError::ID = 0;
-std::string getPGOFuncName(StringRef RawFuncName,
- GlobalValue::LinkageTypes Linkage,
+std::string getPGOFuncName(StringRef Name, GlobalValue::LinkageTypes Linkage,
teresajohnson wrote:
I guess it depends on whether it is intentional that Clang (and Swift
apparently?) use the old name.
@ellishg was there a reason to leave that as is?
If they must use the old one then maybe getFEPGOFuncName. If they should be
transitioned eventually then using "Legacy" makes sense.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -246,11 +246,27 @@ std::string InstrProfError::message() const {
char InstrProfError::ID = 0;
-std::string getPGOFuncName(StringRef RawFuncName,
- GlobalValue::LinkageTypes Linkage,
+std::string getPGOFuncName(StringRef Name, GlobalValue::LinkageTypes Linkage,
teresajohnson wrote:
Oh I missed the fact that one getPGOFuncName interface was left. I am only
seeing 2 invocations of that interface outside of the unittest test. I would be
in favor of doing renames of both getPGOFuncName interfaces in one patch. A
separate NFC patch is a fine option, and keeps this patch just about fixing the
ICP breakage caused by the delimiter change.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -246,11 +246,27 @@ std::string InstrProfError::message() const {
char InstrProfError::ID = 0;
-std::string getPGOFuncName(StringRef RawFuncName,
- GlobalValue::LinkageTypes Linkage,
+std::string getPGOFuncName(StringRef Name, GlobalValue::LinkageTypes Linkage,
minglotus-6 wrote:
> It looks like the most recent version of the patch changes getPGOFuncName to
> getLegacyPGOFuncName for the old format, and uses getIRPGOFuncName for the
> new format.
This is intentional. In this PR, renaming the `getPGOFuncName` (that takes
[stringified
names](https://github.com/llvm/llvm-project/blob/bc4e0c048aa3cd940b0cea787014c7e8680e5040/llvm/lib/ProfileData/InstrProf.cpp#L249)
as argument) to `getLegacyPGOFuncName` is probably fine since it has three
callers.
Prior to this PR both `getPGOFuncName` (that [takes Functions as
argument](https://github.com/llvm/llvm-project/blob/bc4e0c048aa3cd940b0cea787014c7e8680e5040/llvm/lib/ProfileData/InstrProf.cpp#L359))
and `getIRPGOFuncName` are called in quite a few places, so changing both
names would cause too many diffs in this functional change. To use (simpler)
`getPGOFuncName` for new `;` format, I'm thinking a separate NFC patch is the
way to go.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -300,12 +316,8 @@ getIRPGONameForGlobalObject(const GlobalObject &GO,
GlobalValue::LinkageTypes Linkage,
StringRef FileName) {
SmallString<64> Name;
- if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-Name.append(FileName.empty() ? "" : FileName);
-Name.append(";");
- }
Mangler().getNameWithPrefix(Name, &GO, /*CannotUsePrivateLabel=*/true);
teresajohnson wrote:
What I'm confused about is that the function name is already the mangled name,
at least using clang with c++. Is it not for objective C?
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
https://github.com/minglotus-6 edited https://github.com/llvm/llvm-project/pull/74008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [PGO][GlobalValue][LTO]In GlobalValues::getGlobalIdentifier, use semicolon as delimiter for local-linkage varibles. (PR #74008)
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+if [ $# -lt 2 ]; then
+ echo "Path to clang and llvm-profdata required!"
+ echo "Usage: update_icall_promotion_inputs.sh /path/to/updated/clang
/path/to/updated/llvm-profdata"
+ exit 1
+else
+ CLANG=$1
+ LLVMPROFDATA=$2
+fi
+
+# Allows the script to be invoked from other directories.
+OUTDIR=$(dirname $(realpath -s $0))
+
+# Creates trivial header file to expose global_func.
+cat > ${OUTDIR}/lib.h << EOF
+void global_func();
+EOF
+
+# Creates lib.cc. global_func might call one of two indirect callees. Both
+# indirect callees have internal linkage.
minglotus-6 wrote:
good catch. I changed 'callee1' to external linkage in this script and the test
IRs.
https://github.com/llvm/llvm-project/pull/74008
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
