[MemProf] Expand optimization scope to internal linkage function #73236

lifengxiang1025 · 2023-11-23T12:30:11Z

Now MemProf can't do IR annotation right in the local linkage function and global initial function __cxx_global_var_init. In llvm-profdata which convert raw memory profile to memory profile, it uses function name in dwarf to create GUID. But when llvm consumes memory profile, it use getIRPGOFuncName or getPGOFuncName which returns local linkage function as FileName;FunctionName or FileName:FunctionName to get function name and create GUID. So profile creator's GUID is not same as profile consumer.
So I think MemProf should be used with unique-internal-linkage-names and don't use PGOFuncName.
__cxx_global_var_init is created later than where UniqueInternalLinkageNames works. So I add uniq suffix to __cxx_global_var_init additionally.

llvmbot · 2023-11-23T12:30:39Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: None (lifengxiang1025)

Changes

Now MemProf can't do IR annotation right in the local linkage function and global initial function __cxx_global_var_init. In llvm-profdata which convert raw memory profile to memory profile, it uses function name in dwarf to create GUID. But when llvm consumes memory profile, it use getIRPGOFuncName or getPGOFuncName which returns local linkage function as FileName;FunctionName or FileName:FunctionName to get function name and create GUID. So profile creator's GUID is not same as profile consumer.
So I think MemProf should be used with unique-internal-linkage-names and don't use PGOFuncName.
__cxx_global_var_init is created later than where UniqueInternalLinkageNames works. So I add uniq suffix to __cxx_global_var_init additionally.

Full diff: https://github.com/llvm/llvm-project/pull/73236.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGDeclCXX.cpp (+3)
(modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+1-15)

diff --git a/clang/lib/CodeGen/CGDeclCXX.cpp b/clang/lib/CodeGen/CGDeclCXX.cpp
index e08a1e5f42df20c..ae45c23c9e6811c 100644
--- a/clang/lib/CodeGen/CGDeclCXX.cpp
+++ b/clang/lib/CodeGen/CGDeclCXX.cpp
@@ -538,6 +538,9 @@ CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
   {
     llvm::raw_svector_ostream Out(FnName);
     getCXXABI().getMangleContext().mangleDynamicInitializer(D, Out);
+    if (getCodeGenOpts().UniqueInternalLinkageNames) {
+      Out << getModuleNameHash();
+    }
   }
 
   // Create a variable initialization function.
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index c6134ce77136364..97db0122be7c8e7 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -677,24 +677,10 @@ static void readMemprof(Module &M, Function &F,
                         const TargetLibraryInfo &TLI) {
   auto &Ctx = M.getContext();
 
-  auto FuncName = getIRPGOFuncName(F);
+  auto FuncName = F.getName();
   auto FuncGUID = Function::getGUID(FuncName);
   std::optional<memprof::MemProfRecord> MemProfRec;
   auto Err = MemProfReader->getMemProfRecord(FuncGUID).moveInto(MemProfRec);
-  if (Err) {
-    // If we don't find getIRPGOFuncName(), try getPGOFuncName() to handle
-    // profiles built by older compilers
-    Err = handleErrors(std::move(Err), [&](const InstrProfError &IE) -> Error {
-      if (IE.get() != instrprof_error::unknown_function)
-        return make_error<InstrProfError>(IE);
-      auto FuncName = getPGOFuncName(F);
-      auto FuncGUID = Function::getGUID(FuncName);
-      if (auto Err =
-              MemProfReader->getMemProfRecord(FuncGUID).moveInto(MemProfRec))
-        return Err;
-      return Error::success();
-    });
-  }
   if (Err) {
     handleAllErrors(std::move(Err), [&](const InstrProfError &IPE) {
       auto Err = IPE.get();

llvmbot · 2023-11-23T12:30:39Z

@llvm/pr-subscribers-llvm-transforms

Author: None (lifengxiang1025)

Changes

Now MemProf can't do IR annotation right in the local linkage function and global initial function __cxx_global_var_init. In llvm-profdata which convert raw memory profile to memory profile, it uses function name in dwarf to create GUID. But when llvm consumes memory profile, it use getIRPGOFuncName or getPGOFuncName which returns local linkage function as FileName;FunctionName or FileName:FunctionName to get function name and create GUID. So profile creator's GUID is not same as profile consumer.
So I think MemProf should be used with unique-internal-linkage-names and don't use PGOFuncName.
__cxx_global_var_init is created later than where UniqueInternalLinkageNames works. So I add uniq suffix to __cxx_global_var_init additionally.

Full diff: https://github.com/llvm/llvm-project/pull/73236.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGDeclCXX.cpp (+3)
(modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+1-15)

diff --git a/clang/lib/CodeGen/CGDeclCXX.cpp b/clang/lib/CodeGen/CGDeclCXX.cpp
index e08a1e5f42df20c..ae45c23c9e6811c 100644
--- a/clang/lib/CodeGen/CGDeclCXX.cpp
+++ b/clang/lib/CodeGen/CGDeclCXX.cpp
@@ -538,6 +538,9 @@ CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
   {
     llvm::raw_svector_ostream Out(FnName);
     getCXXABI().getMangleContext().mangleDynamicInitializer(D, Out);
+    if (getCodeGenOpts().UniqueInternalLinkageNames) {
+      Out << getModuleNameHash();
+    }
   }
 
   // Create a variable initialization function.
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index c6134ce77136364..97db0122be7c8e7 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -677,24 +677,10 @@ static void readMemprof(Module &M, Function &F,
                         const TargetLibraryInfo &TLI) {
   auto &Ctx = M.getContext();
 
-  auto FuncName = getIRPGOFuncName(F);
+  auto FuncName = F.getName();
   auto FuncGUID = Function::getGUID(FuncName);
   std::optional<memprof::MemProfRecord> MemProfRec;
   auto Err = MemProfReader->getMemProfRecord(FuncGUID).moveInto(MemProfRec);
-  if (Err) {
-    // If we don't find getIRPGOFuncName(), try getPGOFuncName() to handle
-    // profiles built by older compilers
-    Err = handleErrors(std::move(Err), [&](const InstrProfError &IE) -> Error {
-      if (IE.get() != instrprof_error::unknown_function)
-        return make_error<InstrProfError>(IE);
-      auto FuncName = getPGOFuncName(F);
-      auto FuncGUID = Function::getGUID(FuncName);
-      if (auto Err =
-              MemProfReader->getMemProfRecord(FuncGUID).moveInto(MemProfRec))
-        return Err;
-      return Error::success();
-    });
-  }
   if (Err) {
     handleAllErrors(std::move(Err), [&](const InstrProfError &IPE) {
       auto Err = IPE.get();

lifengxiang1025 · 2023-11-23T12:39:06Z

@teresajohnson Can you take a look and provide your insights?

teresajohnson · 2023-11-27T16:31:47Z

@snehasish can you take a look at the issue described here? Should we be doing something different in llvm-profdata?

snehasish · 2023-11-27T23:09:41Z

@lifengxiang1025 thanks for flagging this issue. I think it's best to not rely on unique-internal-linkage-name here. Instead we should extend the logic in RawMemProfReader.cpp to include "filename;" if the function is internal linkage as expected by IRPGOFuncName. Can you try this approach?

llvm-project/llvm/lib/ProfileData/RawMemProfReader.cpp

Lines 509 to 510 in c0fe071

    
           const uint64_t Guid = 
        
               IndexedMemProfRecord::getGUID(DIFrame.FunctionName);

teresajohnson · 2023-11-27T23:50:11Z

@lifengxiang1025 thanks for flagging this issue. I think it's best to not rely on unique-internal-linkage-name here. Instead we should extend the logic in RawMemProfReader.cpp to include "filename;" if the function is internal linkage as expected by IRPGOFuncName. Can you try this approach?

I don't think we want to change the name in the frames themselves, as the names in the debug info when matching don't contain this prefix. Although I suppose we could modify the matcher to add the prefixes when matching frames too. I.e. here:

llvm-project/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

Line 806 in c0fe071

auto CalleeGUID = Function::getGUID(Name);

It sounds like the issue is that the names used to index the memprof records do not use the PGOFuncName scheme and therefore we never find any memprof records for an internal linkage function during matching. I.e. the GUID passed to InstrProfWriter::addMemProfRecord. Unfortunately, looking at RawMemProfReader.cpp it does look like that eventually gets populated out of the Function (GUID) field from same Frame setup code. So we'd either need to do some extra handling in that code so that the prefix is only used for the record entry, or change the matcher (the latter may be the easiest).

lifengxiang1025 · 2023-11-28T03:10:19Z

@lifengxiang1025 thanks for flagging this issue. I think it's best to not rely on unique-internal-linkage-name here. Instead we should extend the logic in RawMemProfReader.cpp to include "filename;" if the function is internal linkage as expected by IRPGOFuncName. Can you try this approach?

llvm-project/llvm/lib/ProfileData/RawMemProfReader.cpp

Lines 509 to 510 in c0fe071

const uint64_t Guid =

IndexedMemProfRecord::getGUID(DIFrame.FunctionName);

I don't find how to get linkage type in dwarf. It seems dwarf don't record linkage type. And the linkage type between thin compile and link step can change also.

snehasish · 2023-11-28T18:48:46Z

change the matcher (the latter may be the easiest).

My response assumed that it was already using the IRPGOFuncName for matching. Yes, I think we should make the switch then.

It seems dwarf don't record linkage type.

Yes, you're right. As an alternative can we use the symbol table and find Bind = LOCAL to add the prefix before hashing?

lifengxiang1025 · 2023-11-29T01:52:25Z

Yes, you're right. As an alternative can we use the symbol table and find Bind = LOCAL to add the prefix before hashing?

If we choose this method. I think we can't deal with the situation which one symbol is not local linkage type in thin compile, but will be converted to local linkage after thin backend by thinlto's internalization. In this situation function name in llvm-profdata will have prefix with file name. But when llvm consumes memory profile, PGOFuncName won't return function name with prefix.

teresajohnson · 2023-11-29T16:56:25Z

Yes, you're right. As an alternative can we use the symbol table and find Bind = LOCAL to add the prefix before hashing?

If we choose this method. I think we can't deal with the situation which one symbol is not local linkage type in thin compile, but will be converted to local linkage after thin backend by thinlto's internalization. In this situation function name in llvm-profdata will have prefix with file name. But when llvm consumes memory profile, PGOFuncName won't return function name with prefix.

If I understand the issue you are describing, that would only occur if the instrumentation build also used ThinLTO. Is that a typical use case for you?

teresajohnson · 2023-11-29T18:03:12Z

@snehasish and I chatted more about this offline. Using the dwarf info to figure out the right source name prefix during indexing is not straightforward. The source file name used as the prefix in the compiler for the IRPGOName is that of the TU. The source file of the line in the dwarf info may be a header. It could be possible to walk up the profiled inline frames to find a source file, but that is error prone in certain circumstances (e.g. if the profiled binary had thinlto cross module inlining, and if it isn't obvious which source file name is a non-included TU).

I think your change to the matching is almost strictly better than the complete non-matching we get currently for local functions. It would even work in many cases without uniq suffixes, but uniq suffixes will make that work even better. So let's go ahead with that change.

However, we'd like to request that you split the __cxx_global_var_init change into a separate patch, as it is somewhat orthogonal to the matching change, and in case there are unexpected issues with that change. Can you split that out and make the test case here for matching of a non __cxx_global_var_init function?

Thanks!

lifengxiang1025 · 2023-11-30T06:46:17Z

@snehasish and I chatted more about this offline. Using the dwarf info to figure out the right source name prefix during indexing is not straightforward. The source file name used as the prefix in the compiler for the IRPGOName is that of the TU. The source file of the line in the dwarf info may be a header. It could be possible to walk up the profiled inline frames to find a source file, but that is error prone in certain circumstances (e.g. if the profiled binary had thinlto cross module inlining, and if it isn't obvious which source file name is a non-included TU).

I think your change to the matching is almost strictly better than the complete non-matching we get currently for local functions. It would even work in many cases without uniq suffixes, but uniq suffixes will make that work even better. So let's go ahead with that change.

However, we'd like to request that you split the __cxx_global_var_init change into a separate patch, as it is somewhat orthogonal to the matching change, and in case there are unexpected issues with that change. Can you split that out and make the test case here for matching of a non __cxx_global_var_init function?

Thanks!

Thanks for your reply. I have removed the __cxx_global_var_init change. Please review again.

llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

Enna1 · 2023-12-01T03:35:38Z

also s/linakge/linkage/ in commit message and PR title.

llvmbot added clang Clang issues not falling into any other category clang:codegen llvm:transforms labels Nov 23, 2023

lifengxiang1025 force-pushed the lfx.memprof branch from 9dd4626 to fdce130 Compare November 23, 2023 12:32

Enna1 requested a review from teresajohnson November 24, 2023 02:16

lifengxiang1025 force-pushed the lfx.memprof branch from fdce130 to 82cebba Compare November 27, 2023 07:06

llvmbot added the PGO Profile Guided Optimizations label Nov 27, 2023

Enna1 requested a review from snehasish November 27, 2023 07:31

lifengxiang1025 force-pushed the lfx.memprof branch 2 times, most recently from f91e72a to 3e50c55 Compare November 30, 2023 06:40

teresajohnson reviewed Nov 30, 2023

View reviewed changes

llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh Outdated Show resolved Hide resolved

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp Show resolved Hide resolved

lifengxiang1025 force-pushed the lfx.memprof branch from 3e50c55 to fd0f4ff Compare December 1, 2023 03:25

[MemProf] Expand optimization scope to internal linkage function

1425bf3

lifengxiang1025 force-pushed the lfx.memprof branch from fd0f4ff to 1425bf3 Compare December 1, 2023 03:44

lifengxiang1025 changed the title ~~[MemProf] Expand optimization scope to internal linakge function~~ [MemProf] Expand optimization scope to internal linkage function Dec 1, 2023

teresajohnson approved these changes Dec 1, 2023

View reviewed changes

Enna1 merged commit 340cb19 into llvm:main Dec 1, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MemProf] Expand optimization scope to internal linkage function #73236

[MemProf] Expand optimization scope to internal linkage function #73236

lifengxiang1025 commented Nov 23, 2023

llvmbot commented Nov 23, 2023 •

edited

llvmbot commented Nov 23, 2023

lifengxiang1025 commented Nov 23, 2023 •

edited

teresajohnson commented Nov 27, 2023

snehasish commented Nov 27, 2023 •

edited

teresajohnson commented Nov 27, 2023

lifengxiang1025 commented Nov 28, 2023

snehasish commented Nov 28, 2023

lifengxiang1025 commented Nov 29, 2023 •

edited

teresajohnson commented Nov 29, 2023

teresajohnson commented Nov 29, 2023

lifengxiang1025 commented Nov 30, 2023

Enna1 commented Dec 1, 2023

[MemProf] Expand optimization scope to internal linkage function #73236

[MemProf] Expand optimization scope to internal linkage function #73236

Conversation

lifengxiang1025 commented Nov 23, 2023

llvmbot commented Nov 23, 2023 • edited

llvmbot commented Nov 23, 2023

lifengxiang1025 commented Nov 23, 2023 • edited

teresajohnson commented Nov 27, 2023

snehasish commented Nov 27, 2023 • edited

teresajohnson commented Nov 27, 2023

lifengxiang1025 commented Nov 28, 2023

snehasish commented Nov 28, 2023

lifengxiang1025 commented Nov 29, 2023 • edited

teresajohnson commented Nov 29, 2023

teresajohnson commented Nov 29, 2023

lifengxiang1025 commented Nov 30, 2023

Enna1 commented Dec 1, 2023

llvmbot commented Nov 23, 2023 •

edited

lifengxiang1025 commented Nov 23, 2023 •

edited

snehasish commented Nov 27, 2023 •

edited

lifengxiang1025 commented Nov 29, 2023 •

edited